Skip to content
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

fix: transition eventManager errors #19835

Merged
merged 3 commits into from
Jan 24, 2022
Merged

fix: transition eventManager errors #19835

merged 3 commits into from
Jan 24, 2022

Conversation

marktnoonan
Copy link
Contributor

@marktnoonan marktnoonan commented Jan 22, 2022

Two small fixes:

  1. Wrapper div in Runner.vue so there is a HTML root element for the transition. Navigation to other pages from Spec Runner works normally now.
  2. Testing navigating from a spec, to the rest of the app, and then to another spec, I saw an event manager error and the new spec would not load. On the 2nd visit to the Spec Runner, we were trying to get the eventManager before it was initialized. Resetting the value of the initialized ref when we unmount the runner component fixes that, because the container component uses v-if="initialized" to decide when to render the runner itself, so if it stays true, we render too early on subsequent visits.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jan 22, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Jan 22, 2022



Test summary

4351 0 51 0Flakiness 0


Run details

Project cypress
Status Passed
Commit d8b1e97
Started Jan 24, 2022 2:12 AM
Ended Jan 24, 2022 2:25 AM
Duration 13:12 💡
OS Linux Debian - 10.10
Browser Electron 94

View run in Cypress Dashboard ➡️


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

@brian-mann
Copy link
Member

cc @lmiller1990 as this sounds related to work we did previously together regarding initialization and memoizing initialization of the runner code

@@ -15,6 +15,7 @@ export function useUnifiedRunner () {

onBeforeUnmount(() => {
UnifiedRunnerAPI.teardown()
initialized.value = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, seems like a good place to do this

@lmiller1990
Copy link
Contributor

I wonder if we can add a basic test of sorts, eg - navigate to the runner, choose a spec, navigate away? IIRC that was breaking when we tried last week, and since this fixes that problem, we could also add a test for it (I think we have the infrastructure to facilitate this kind of test now)?

@lmiller1990 lmiller1990 merged commit 5cac74c into 10.0-release Jan 24, 2022
@lmiller1990 lmiller1990 deleted the 10.0-transition branch January 24, 2022 02:34
@brian-mann
Copy link
Member

brian-mann commented Jan 24, 2022

Yes, we have the infra now. I wouldn’t have merged this PR without tests. Please add them. No PRs should be going in without tests. That’s just asking for regressions to trip us up later.

@marktnoonan
Copy link
Contributor Author

Yes, just checked and with the new Cypress in Cypress, we can visit the spec, then go to Settings or Runs and go back to the Specs List and click in to see the spec again. Seems like there is a lot of great stuff we'll be able to do with this! I'll make a PR with the new tests shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants