fix: running a new test after already having run tests #21087
Conversation
|
Thanks for taking the time to open a PR!
|
|
Test summary
Run details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
||||||||||||||||||||||||||
…eady-running-test
| const specs = computed(() => { | ||
| return props.gql.currentProject?.specs ?? [] | ||
| }) | ||
|
|
||
| watchSpec(specs) |
There was a problem hiding this comment.
Not a strong opinion but I like to functions that call watch or watchEffect in the actual component file to give visibility, or you need to dig through a bunch of other files to find the watcher. It's like when you are trying to track down a useState or useEffect nested deeply in a hook in React.
I don't mind too much, but something about watchSpec(specs) is more readable to me than useUnifiedRunner(specs). The former is more clear that something reactive will happen. Happy to field opinions and not really a big deal either way.
There was a problem hiding this comment.
I'd prefer adding 'watcher' to the composable's name in some way if more discoverability is desired. Putting the responsibility of activating the watchers on the consuming component just leaves a logic hole where there doesn't need to be one.
|
|
||
| if (selectorPlaygroundStore.show) { | ||
| const autIframe = getAutIframeModel() | ||
| watch(() => getPathForPlatform(route.query.file as string), (newQueryFile) => { |
There was a problem hiding this comment.
We have two things watching route.query.file, line 26 and line 44. I wonder if we should combine them so all the watching of the same variable is in the same function? There's an argument of "one watch per effect" so again I don't mind either way, but wanted to point this out since you might not have noticed the double watcher.
I'm guessing the perf overhead is negligible so just wanted to call this out to see if it was intentional or not.
I tried the single watcher and it seems to work just fine, too: #21112
There was a problem hiding this comment.
For me, moving this in here prevents the selector playground from ever opening - opening it triggers the watchEffect due to the value changing in the store, and the watchEffect closes it up right away.
There was a problem hiding this comment.
Yeah I played around with combining them at one point and ran into what @marktnoonan is talking about. Specifically I think it's these tests failing on #21112: https://app.circleci.com/pipelines/github/cypress-io/cypress/36466/workflows/828e7886-b042-41de-8753-e693c6f9a292/jobs/1460784
There may be a way to tweak the logic to fix that, but ultimately, I'm ok with keeping them separate since I do like the idea of "one watch per effect"
There was a problem hiding this comment.
"one watch per effect" seems like a reasonable approach to take until it becomes a performance issue. I prefer the sensible encapsulation over worrying about the lifecycle too much.
There was a problem hiding this comment.
Nice, having tested a bit this works well. The only drawback to the watchEffect that I can think of is that it runs more times than strictly necessary, since it sets values that it also watches. But there don't seem to be any negative side effects of that, and the simplicity of gathering this all into the composable is nice. I just left one comment about an unused param.
As for the problem itself:
Previously, we had a watcher that was only watching the routing changes in order to set the active spec. The problem is that this was caching the specs list so if we had a stale specs list it wasn't going to be able to find the newly added spec.
I was super curious about this because I couldn't grok why it would be happening, so I looked around a little bit. It seems that the route watcher's callback was firing twice - once with a reference to the correct specs list, exactly as we'd want, and then again with the out of date specs list, and it's that second run that triggers the redirect + error.
The watcher runs twice because when you first visit the spec runner calls getPathForPlatform for the new and old values of route.query.file. In running the watcher's callback for the old value (which is just undefined, then null), Vue is using the old value of the specs list, but somehow that callback runs second. So, before this PR, we briefly passed through a correct "spec found" state before it gets overwritten by a "not found" state and bounces us to the specs page. That's probably an oversimplification but I'm trying to avoid going down a rabbit hole.
We could fix the issue by making sure the watcher doesn't run twice with a coupe of checks on the new/old value parameters, I tried it out and it works fine, but I still feel the changes in this PR make it all more readable (to @lmiller1990's point).
I'd be fine with useUnifiedRunnerAndWatchers or something as the name to address the discoverability of these watchers.
I'm sure we can tighten this up even further at some point but it's probably not very valuable to do that unless we see perf issues.
|
|
||
| if (selectorPlaygroundStore.show) { | ||
| const autIframe = getAutIframeModel() | ||
| watch(() => getPathForPlatform(route.query.file as string), (newQueryFile) => { |
There was a problem hiding this comment.
We can remove this unused param:
| watch(() => getPathForPlatform(route.query.file as string), (newQueryFile) => { | |
| watch(() => getPathForPlatform(route.query.file as string), () => { |
There was a problem hiding this comment.
Removed: cypress-io/cypress@98c813d (#21087)
|
@ryanthemanuel It looks like moving it into the Video shows us doing the right behavior, but the order of things has changed. |
@marktnoonan, hmmm it does appear to be flaky (I can't get it to fail locally) but I'll see if I can figure out why. |
lmiller1990
deleted the
ryanm/fix/running-new-test-after-already-running-test
branch
…e-config * 10.0-release: chore: Move component-index generation to scaffold-config package (#21090) fix: label text should be clickable to toggle snapshot highlight (#21122) feat: add next preset to webpack-dev-server-fresh (#21069) chore: add dev-servers as deps to server to be included in the binary (#21142) fix: do not highlight preExtension if selected option is renameFolder (#21121) fix(unify): Remove 'Reconfigure' dropdown from Testing Type chooser (#21063) feat(unify): launchpad header breadcrumbs and reusable tooltip component (#20648) test: add windows-test-kitchensink job (#20847) fix: aut centering and height calculation (#21019) chore: fix tests that fail on windows (#21055) fix: running a new test after already having run tests (#21087) fix: ct project setup framework keys for next and nuxt (#21116) fix: remove MountReturn from scaffolded ct support file (#21119) fix: do not scaffold fixtures if folder exist (#21078) fix: revert "fix: types for Cypress.Commands.add (#20376)" (#21104) chore: Update Chrome (stable) to 100.0.4896.127 and Chrome (beta) to 101.0.4951.34 (#21083) fix: types for Cypress.Commands.add (#20376) (#20377)
User facing changelog
Users can now execute a test in open mode, go back to the specs page, add a new test, and run that newly added test without error
Additional details
Previously, we had a watcher that was only watching the routing changes in order to set the active spec. The problem is that this was caching the specs list so if we had a stale specs list it wasn't going to be able to find the newly added spec. We fix this by ensuring that any active spec management is done inside of a watchEffect so that we are watching changes to all things relevant.
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?cypress.schema.json?