fix: running a new test after already having run tests #21087
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,15 +1,14 @@ | ||||||
| import { Ref, onMounted, ref, watch, watchEffect, onBeforeUnmount, readonly } from 'vue' | ||||||
| import { getAutIframeModel, UnifiedRunnerAPI } from '../runner' | ||||||
| import { useSpecStore } from '../store' | ||||||
| import { useSelectorPlaygroundStore } from '../store/selector-playground-store' | ||||||
| import type { SpecFile } from '@packages/types/src' | ||||||
| import { useRoute } from 'vue-router' | ||||||
| import { getPathForPlatform } from '../paths' | ||||||
|
|
||||||
| const initialized = ref(false) | ||||||
|
|
||||||
| export function useUnifiedRunner (specs: Ref<ReadonlyArray<SpecFile>>) { | ||||||
| onMounted(async () => { | ||||||
| await UnifiedRunnerAPI.initialize() | ||||||
| initialized.value = true | ||||||
|
|
@@ -20,45 +19,39 @@ export function useUnifiedRunner () { | |||||
| initialized.value = false | ||||||
| }) | ||||||
|
|
||||||
| const specStore = useSpecStore() | ||||||
| const route = useRoute() | ||||||
| const selectorPlaygroundStore = useSelectorPlaygroundStore() | ||||||
|
|
||||||
| watchEffect(() => { | ||||||
| const queryFile = getPathForPlatform(route.query.file as string) | ||||||
|
|
||||||
| if (!queryFile) { | ||||||
| // no file param, we are not showing a file | ||||||
| // so no action needed when specs list updates | ||||||
| return | ||||||
| } | ||||||
|
|
||||||
| const activeSpecInSpecsList = specs.value.find((x) => x.relative === queryFile) | ||||||
|
|
||||||
| if (activeSpecInSpecsList && specStore.activeSpec?.relative !== activeSpecInSpecsList.relative) { | ||||||
| specStore.setActiveSpec(activeSpecInSpecsList) | ||||||
| } else if (!activeSpecInSpecsList) { | ||||||
| specStore.setActiveSpec(null) | ||||||
| } | ||||||
| }) | ||||||
|
|
||||||
| watch(() => getPathForPlatform(route.query.file as string), (newQueryFile) => { | ||||||
|
There was a problem hiding this comment. We can remove this unused param:
Suggested change
There was a problem hiding this comment. Removed: cypress-io/cypress@ There was a problem hiding this comment. We have two things watching 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 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. |
||||||
| if (selectorPlaygroundStore.show) { | ||||||
| const autIframe = getAutIframeModel() | ||||||
|
|
||||||
| autIframe.toggleSelectorPlayground(false) | ||||||
| selectorPlaygroundStore.setEnabled(false) | ||||||
| selectorPlaygroundStore.setShow(false) | ||||||
| } | ||||||
| }, { flush: 'post' }) | ||||||
|
|
||||||
| return { | ||||||
| initialized: readonly(initialized), | ||||||
| } | ||||||
| } | ||||||
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.
Not a strong opinion but I like to functions that call
watchorwatchEffectin 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 auseStateoruseEffectnested deeply in a hook in React.I don't mind too much, but something about
watchSpec(specs)is more readable to me thanuseUnifiedRunner(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.