-
Notifications
You must be signed in to change notification settings - Fork 190
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
chore(e2e): refactor context to use yargs parser #6398
Conversation
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.
GitHub is collapsing this one because of the amount of changes, the logic is all the same, but yeah, rewriting it from env vars changed a lot of code here, so might be easier to just read through it as if this file was just added
Partial<DesktopParsedArgs & WebParsedArgs>; | ||
|
||
export function isTestingDesktop(ctx = context): ctx is DesktopParsedArgs { | ||
return testEnv === 'desktop'; |
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 can explicitly check for every desktop specific command argument here as defined above, but yargs with strict()
guarantees that this assertion is correct if we managed to set testEnv
to desktop
(same assumption can be made for web)
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 this specific case it's unlikely that the assertion will be broken, but I wonder if there is a way to make the assertion less disconnected from the code that actually ensures this is valid
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 was thinking something like a map of the testEnv -> argsParser, which would be used to in both pieces of code. but looking a bit deeper into how the types are build, it's probably more complicated to implement that and probably not worth it given that the very small risk.
@@ -26,6 +26,9 @@ | |||
], | |||
"js-yaml": [ | |||
"^3.13.1" | |||
], | |||
"yargs": [ | |||
"^4.8.1" |
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.
hadron-build is on 4 and I don't feel like I can deal with it as a drive by of adding yargs usage to compass-e2e-tests tbh
f9a5867
to
f988cb8
Compare
f988cb8
to
ea818b7
Compare
@@ -76,7 +71,7 @@ export async function mochaGlobalSetup(this: Mocha.Runner) { | |||
|
|||
debug('X DISPLAY', process.env.DISPLAY); | |||
|
|||
if (!DISABLE_START_STOP) { | |||
if (!context.disableStartStop) { |
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.
now that the params are camelCase, I'd kind of expect them to also follow the naming convention and this bool to be isStartStopDisabled
. might be just me and others would expect the naming to be direct derivative of the original param 🤷 What do you think? If we wanted to rename them, we could do that with yargs middleware callback https://yargs.js.org/docs/#api-reference-middlewarecallbacks-applybeforevalidation
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 think being able to map arguments that are passed to the script to the usage in the code is more important for the dev code like the e2e tests. Middleware would add one more extra step to this process and this helper doesn't extend the types, so it would be even extra code in between to align the types with the remapping. That's why I'd prefer not to change it
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.
Neat! ❤️ Left a suggestion, otherwise looks great!
This patch continues to clean-up compass-e2e-tests code so that it's hopefully easier to maintain going forward. We kinda outgrew configuring the tests using just env vars and manually checking for some arg flags, so this patch refactors all this logic to use yargs parser to better structure the logic around providing various flags, especially for cases where we expect some of them to be provided together or not overlap.
We basically have two very distinct environments for testing, web or desktop, with some subgroups that can be triggered with args (packaged vs just compiled, atlas cloud vs local sandbox, with sandbox with cloud login coming soon) + a common set of args that's applied in both cases, this is the logic that's now coded as part of yargs argv descriptor. This patch also adds helper methods to make it easier and stricter (both in types and runtime) to identify the environment the tests are running against.
This is how the help output looks for the script now