feat(smoke-tests): test updating from the latest release to the newly packaged app COMPASS-8532 COMPASS-8535 #6669
Conversation
lerouxb
force-pushed
the
autoupdate-to
branch
from
563dd83 to
3c7162d
Compare
| console.log( | ||
| 'pause to make sure the app properly exited before starting again' | ||
| ); | ||
| await wait(10_000); |
There was a problem hiding this comment.
Could we poll the ps with some pid instead? 🤔 Ideally from within the cleanup function. These timeouts always makes me nervous and often adds unneeded delays.
There was a problem hiding this comment.
Yeah, but where do we get the pid? I think we can clean it up but in an effort to get somewhere let's do that as a follow-up.
| Selectors.AutoUpdateReleaseNotesLink | ||
| ); | ||
| await releaseNotesLink.waitForDisplayed(); | ||
| // for now we only know the new version in the auto-update-to case |
There was a problem hiding this comment.
Would this be easier to get to now that we're running the update server from within the CLI process?
There was a problem hiding this comment.
Maybe. Haven't thought of it. But if we do I'd just do it in a follow-up.
|
|
||
| export function assertCommonBuildInfo( | ||
| buildInfo: unknown | ||
| ): asserts buildInfo is CommonBuildInfo { | ||
| assertObjectHasKeys(buildInfo, 'buildInfo', commonKeys); | ||
| assert( | ||
| SUPPORTED_CHANNELS.includes(buildInfo.channel as Channel), |
| const installerPath = | ||
| testName === 'auto-update-to' | ||
| ? await getLatestRelease( | ||
| channel, | ||
| context.arch, | ||
| kind, | ||
| context.forceDownload | ||
| ) | ||
| : filepath; |
There was a problem hiding this comment.
Won't this depend on the order if multiple tests are passed to the CLI?
I suggest we make this a bit less powerful and run only one test per invocation of the CLI (effectively changing context.tests to context.test) to make it easier to reason about what installer needs to run first.
There was a problem hiding this comment.
Looking at this again, it does feel a bit misplaced here, instead of being a part of the actual test function, which would increase the cohesion 🤔
There was a problem hiding this comment.
OK what I've done now is move all this stuff into the tests. I also changed it so that auto-update-to does not unnecessarily download the newly packaged app when it is going to be installing the latest release anyway.
Let me know what you think.
| version, | ||
| bucketKeyPrefix, | ||
| }: RunUpdateToTestOptions) { | ||
| process.env.PORT = '0'; // dynamic port |
There was a problem hiding this comment.
While this works, this isn't type-safe and we would ideally be passing this through the default function exported by the update server package, which would then be passed to the UpdateChecker constructor deep merging with any default config values:
To be honest, it feels the way config is passed through environment variables here are a remains of the fact that this was invoked as a sub-process earlier.
There was a problem hiding this comment.
yeah it is a bit janky at the moment. That's not where the port is even used, though. update-checker is not the server itself. I think we can make a PR to compass-mongodb-com to allow us to clean it up, then another one here to use that. I'd do it as a follow-up, though, so we can have the tests running in the meantime.
Co-authored-by: Kræn Hansen <[email protected]>
Unfortunately because we skip the update tests on mac in CI and we don't have the windows .exe installer done yet (needed when installing the latest release windows), this won't actually run in CI yet.