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: Subscribe to framework detection changes in wizard #26437

Merged
merged 4 commits into from
Apr 10, 2023

Conversation

mike-plummer
Copy link
Contributor

Additional details

Launchpad does not wait for the data context to initialize in case there are significant slowdowns which would make the UI non-responsive on launch. This can cause the CT framework auto-detection to not populate if the detection is somehow delayed (complex project, slow machine, etc) - the detection occurs correctly but the GQL query in the Vue component has already resolved so the result is not reflected in the UI.

This PR adds a subscription so the Vue component can listen for updates to this data and reflect them in the UI.

Steps to test

  1. On develop, add a delay to wizard initialization by updating the initialize function with this line:
// WizardActions.ts
async initialize () {
  await new Promise((resolve) => setTimeout(resolve, 5000))
  ...
  1. Create a new empty CT project: npm create vite@latest, use any supported framework & options
  2. Install project dependencies and add Cypress: cd <project dir>, npm i, npm i -D cypress
  3. Open project directly into CT mode: cd <cypress repo> , yarn cypress:open --project <path to project> --component
  4. Observe that UI does not detect framework/bundler, stays on "Pick a framework"
  5. Switch to this branch, repeat procedure
  6. Observe framework & bundler are reflected in the UI after small delay

How has the user experience changed?

Before

before.mov

After

after.mov

PR Tasks

@cypress
Copy link

cypress bot commented Apr 5, 2023

25 flaky tests on run #45414 ↗︎

0 26909 1281 0 Flakiness 25

Details:

trigger ci
Project: cypress Commit: cddbd4a01c
Status: Passed Duration: 18:54 💡
Started: Apr 10, 2023 9:08 PM Ended: Apr 10, 2023 9:27 PM
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-electron

View Output Video

Test Artifacts
... > correctly returns currentRetry Output Video
... > correctly returns currentRetry Output Video
... > correctly returns currentRetry Output Video
Flakiness  create-from-component.cy.ts • 1 flaky test • app-e2e

View Output Video

Test Artifacts
... > runs generated spec Output Screenshots Video
Flakiness  specs_list_latest_runs.cy.ts • 2 flaky tests • app-e2e

View Output Video

Test Artifacts
App/Cloud Integration - Latest runs and Average duration > when no runs are recorded > shows placeholders for all visible specs Output Screenshots Video
App/Cloud Integration - Latest runs and Average duration > when offline > shows offline alert then hides it after coming online Output Screenshots Video
Flakiness  cypress-origin-communicator.cy.ts • 1 flaky test • app-e2e

View Output Video

Test Artifacts
Cypress In Cypress Origin Communicator > cy.origin passivity with app interactions > passes upon test reload mid test execution Output Screenshots Video
Flakiness  commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-firefox

View Output Video

Test Artifacts
network stubbing > intercepting request > can delay and throttle a StaticResponse Output

The first 5 flaky specs are shown, see all 12 specs in Cypress Cloud.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@lmiller1990
Copy link
Contributor

Mine shows up immediately regardless, even if I have the setTimeout. 🤔 Any ideas?

bug.mov

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Code looks good but I could not run the test plan - any ideas what I might be missing? Is there another step?

@mike-plummer
Copy link
Contributor Author

Code looks good but I could not run the test plan - any ideas what I might be missing? Is there another step?

@lmiller1990 Try yarn dev instead of yarn cypress:open? I was probably running against previously-built state when putting that test procedure together; IIRC cypress:open doesn't trigger any rebuild

@mike-plummer mike-plummer requested a review from a team April 7, 2023 14:05
Copy link
Collaborator

@jordanpowell88 jordanpowell88 left a comment

Choose a reason for hiding this comment

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

The code looks good to me but the after video is less than ideal UX to me. Nothing indicates that we are doing work to search for your framework. Do we have a less disruptive way of alerting the user we are loading something after the initial paint?

@mike-plummer
Copy link
Contributor Author

The code looks good to me but the after video is less than ideal UX to me. Nothing indicates that we are doing work to search for your framework. Do we have a less disruptive way of alerting the user we are loading something after the initial paint?

@jordanpowell88 The five second delay is just to accentuate this scenario and make it more observable - in reality, this typically occurs within hundreds of milliseconds and occasionally happens to just miss the point where the gql query is resolved for the vue component. Unless the user is on a massively slow machine this shouldn't ever be observed as anything more than a "blip"

@jordanpowell88
Copy link
Collaborator

The code looks good to me but the after video is less than ideal UX to me. Nothing indicates that we are doing work to search for your framework. Do we have a less disruptive way of alerting the user we are loading something after the initial paint?

@jordanpowell88 The five second delay is just to accentuate this scenario and make it more observable - in reality, this typically occurs within hundreds of milliseconds and occasionally happens to just miss the point where the gql query is resolved for the vue component. Unless the user is on a massively slow machine this shouldn't ever be observed as anything more than a "blip"

Ok that makes sense. I must have missed this context

@lmiller1990
Copy link
Contributor

I tested locally, works great 💯

@lmiller1990 lmiller1990 self-requested a review April 9, 2023 23:46
@@ -158,6 +158,10 @@ export = {
},

async run (options: LaunchArgs, _loading: Promise<void>) {
// Note: We do not await the `_loading` promise here since initializing
// the data context can significantly delay initial render of the UI
// https://github.com/cypress-io/cypress/issues/26388#issuecomment-1492616609
Copy link
Member

Choose a reason for hiding this comment

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

This is good context, but might be worth pointed to code vs a github comment in the event this is code ever changes so this stays "true" and/or relevant.

Copy link
Contributor

@marktnoonan marktnoonan left a comment

Choose a reason for hiding this comment

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

Works well, and code looks good

@mike-plummer mike-plummer merged commit fdb5642 into develop Apr 10, 2023
@mike-plummer mike-plummer deleted the mikep/26388-detect-framework-subscription branch April 10, 2023 22:34
astone123 pushed a commit to kgroat/cypress that referenced this pull request Apr 19, 2023
tgriesser added a commit that referenced this pull request Apr 25, 2023
* feat/protocol: (45 commits)
  chore: adding support for url:changed (#26519)
  chore: adding viewport:changed to protocol (#26508)
  chore: Reduce dependencies and binary size, add circle ci detector (#26522)
  chore: 12.10.0 release (#26517)
  test: fix flaky tests (#26505)
  chore: Check project dependencies for CT compatibility (#26497)
  chore: update vm2 to 3.9.16 (#26489)
  chore: enable builds on feat/protocol branch (#26506)
  chore: [skip ci] update to labels looked at by stalebot (#26496)
  chore: connecting to electron browser (#26471)
  chore: [skip ci] turning on stale bot (#26488)
  chore: fix issue with logs without wallClockUpdatedAt (#26473)
  Update triage_add_to_project.yml
  chore: Update Chrome (stable) to 112.0.5615.49 and Chrome (beta) to 113.0.5672.24 (#26434)
  feat: display framework definition errors (#26183)
  fix: correctly resolve dependencies for CT onboarding when using Yarn Plug n Play (#26452)
  fix: Subscribe to framework detection changes in wizard (#26437)
  fix: make clicks on type('{enter}') composed (#26395)
  chore: update add-to-project workflow (#26439)
  chore: Pass telemetry resources from the node process to the browser (#26468)
  ...
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.

CT framework detection can fail due to open mode not waiting for DataContext to initialize
5 participants