-
Notifications
You must be signed in to change notification settings - Fork 3.4k
feat: switch browser runner #19048
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: switch browser runner #19048
Conversation
Thanks for taking the time to open a PR!
|
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.
Few small comments
async getCurrentSpecByAbsolute (projectRoot: string, absolute: string) { | ||
// TODO: should cache current specs so we don't need to | ||
// call findSpecs each time we ask for the current spec. | ||
const specs = await this.findSpecs(projectRoot, null) |
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.
Can we just use this.ctx.currentProject.specs
?
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.
Can do!
t.liveMutation('launchOpenProject', { | ||
description: 'Launches project from open_project global singleton', | ||
args: { | ||
specPath: stringArg(), |
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 this nonNull(stringArg())
?
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.
No, since this mutation is used in other places that don't require routing to a specific spec.
const spec: Cypress.Spec = { | ||
// launchProject expects a spec when opening browser for url navigation. | ||
// We give it an empty spec if none is passed so as to land on home page | ||
const emptySpec: Cypress.Spec = { |
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 wonder if we still need this? You could try removing it and see what happens - I don't know if this is necessary now we launch into unified (no need for a fake spec url to visit).
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 still need this. It could be removed but it would require refactoring a chunk of server code that is outside the scope of this PR.
|
||
return specs.filter((spec) => spec.specType === specType) | ||
} | ||
project.specs = !specType ? specs : specs.filter((spec) => spec.specType === specType) |
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.
Wait, why are we mutating project.specs
?
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 currentProject.specs
field was never being set, so they had to be set somewhere. I'll revert to just using findSpecs
for the getCurrentSpecByAbsolute
function. There will be some changes to how specs are managed here soon, we can tackle the inefficiencies soon
* 10.0-release: refactor: remove @packages/desktop-gui (#19127) feat: switch browser runner (#19048) fix: bump resource class for tests that require node_modules install (#19079) fix(unify): reporter styles (#19034) fix test feat(unify): add number of matches to specs search (#19076) feat(launchpad): open in IDE modal and feature (#18975) fix: 10.0 appveyor updateyaml (#19074)
User facing changelog
Allows the changing of the browser from within the runner while keeping the current spec active.
Additional details
We didn't have the functionality of routing to a spec, so I added it. Since the convention of running a spec with the app open is
/runner?file={relativePathToSpec}
, the server changes accommodate for this convention by flagging the new routing underprocess.env.LAUNCHPAD
.Screen.Recording.2021-11-22.at.1.57.08.PM.mov
I'm not sure how to test this thoroughly, since spying on mutations inside component tests don't work and writing an e2e test that targets the runner page causes all sorts of issues (another instance of Cypress causes the reporter and AUT to switch).
PR Tasks