-
Notifications
You must be signed in to change notification settings - Fork 188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(e2e): Smoke tests task & initial entrypoint COMPASS-8531 #6571
Conversation
depends_on: 'package-ubuntu', | ||
}, | ||
|
||
// { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I have most platforms commented out because this is a lot of new tasks that will do nothing for the moment.
@@ -180,6 +214,19 @@ buildvariants: | |||
<% } %> | |||
<% } %> | |||
|
|||
<% for (const buildVariant of SMOKETEST_BUILD_VARIANTS) { %> | |||
<% for (const distribution of ['compass']) { %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looping through just compass here rather than all of COMPASS_DISTRIBUTIONS. For now. Until we want to test readonly and isolated.
@@ -424,6 +471,22 @@ tasks: | |||
- func: save-all-artifacts | |||
vars: | |||
compass_distribution: <%= distribution %> | |||
<% } %> | |||
|
|||
<% for (const distribution of ['compass']) { %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not looping through COMPASS_DISTRIBUTIONS here yet.
@@ -12,6 +12,11 @@ echo "NPM_VERSION: $NPM_VERSION" | |||
echo "APPDATA: $APPDATA" | |||
echo "PATH: $PATH" | |||
|
|||
# these are super useful if you want to run the smoke tests locally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to need these when we want to test with the files produced by a PR or beta or ga build, so I'm just adding it next to other useful/essential variables being output. They aren't secrets so that's fine.
} | ||
} | ||
|
||
if (!(argv.isWindows || argv.isOSX || argv.isUbuntu || argv.isRHEL)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could detect these as a default, but I'm not sure we want to duplicate the logic?
I spent way too much time thinking about this and going back and forth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also check to ensure these are mutually exclusive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about that. Either make them mutually exclusive or allow you to "OR" them together. Then you can use this to download everything if you want to or at least test multiple .zip files at a time. Sure it will break later, but much of the code could run on mac for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I just tested this and it won't work because hadron-build info can only do one at a time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just count them now as a poor man's logical XOR.
@@ -1,7 +1,4 @@ | |||
{ | |||
"extends": "@mongodb-js/tsconfig-compass/tsconfig.common.json", | |||
"compilerOptions": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Driveby from uncommitted changes.
|
||
const handler = function handler(argv) { | ||
cli.argv = argv; | ||
run(argv).catch(abortIfError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end I'm not using download because as is it would download all the packages and therefore mean we have to depend on every package task rather than just the relevant one for the platform.
I could modify download more to filter but we already have the info we need from hadron-build info anyway so I just download straight in the script.
But I think the change here is harmless and a very minor improvement anyway.
verifyPackagesExist(packages); | ||
} | ||
|
||
// TODO(COMPASS-8533): extract or install each package and then test the Compass binary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the epic goes here :)
const extension = config.extension; | ||
|
||
return names | ||
.filter((name) => !extension || name.endsWith(extension)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By optionally filtering the files we'll test we can easily run this on just one file at a time during development and we can also change the evergreen config to add one task per file if we wanted to at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a few minor suggestions, but I like the approach.
The only bigger thing is: I have a feeling "smoke test" is too generic for what we're doing with this project. I mean, this will specifically test "installs" and "updating", right?
const fileWriter = createWriteStream(targetFile).on('finish', () => { | ||
resolve(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you'd want to register an 'error' listener here as well (calling reject
), in case the write stream fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that won't hurt. I find streams such a footgun. There really should be builtin utilities somewhere for these cases. We use the download
npm package in a few other places but it hasn't been published to in 5 years, supports no ESM, no TypeScript.. with no obvious perfect alternative I could quickly find.
} | ||
} | ||
|
||
if (!(argv.isWindows || argv.isOSX || argv.isUbuntu || argv.isRHEL)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also check to ensure these are mutually exclusive?
This is also documented by yargs and in comments, but for convenience I'm adding a summary here.
For running locally and testing with compass you packaged yourself:
For running locally and testing against compass that was packaged by evergreen for a pull request: