-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: Component Testing banner #26625
Conversation
4 flaky tests on run #46056 ↗︎
Details:
commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-electron
cypress/cypress.cy.js • 3 flaky tests • 5x-driver-electron
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
packages/app/src/specs/banners/ComponentTestingAvailableBanner.vue
Outdated
Show resolved
Hide resolved
@@ -20,8 +20,9 @@ | |||
}, | |||
"dependencies": {}, | |||
"devDependencies": { | |||
"@cypress-design/vue-icon": "0.20.0", | |||
"@cypress-design/vue-statusicon": "0.3.0", | |||
"@cypress-design/vue-button": "0.9.2", |
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.
🎉
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.
Minor comments, I will test it now.
onMounted(() => { | ||
updateBannerState('lastShown') | ||
onMounted(async () => { | ||
await updateBannerState('lastShown') |
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 don't think this await
actually does anything - the hooks are async, onMounted
will run the execution will go on without waiting. Was this just a style change?
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.
Mostly stylistic - I made the updateBannerSync
function async, so flowed the await
up through all usages just so the Promise-nature isn't lost. Probably a no-op for now, but if additional logic is added to onMounted
in the future it will at least execute in order within the callback
@@ -100,6 +100,14 @@ export class WizardActions { | |||
|
|||
this.resetWizard() | |||
|
|||
await this.initializeFramework() |
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.
Is this somehow related to this feature, or is it just a drive by bug fix?
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 pulled out the new initializeFramework
action so we could trigger the existing "framework & bundler detection" logic while in e2e mode without touching other wizard stuff. I probably could have just used the existing initialize
action since all it does is a reset then this logic, but I wanted to have a clean action in case there were updates to the wizard initialization flow in the future that we wouldn't want to fire while in e2e
It worked I found it was not working - I forgot to do I did the full workflow and got this, which is expected, since I don't have a CLI installed in the project Looks like it works to me! Curious to track how many people will see this banner. |
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 am still working on actually setting up a project of my own to test, but wanted to share these comments.
The big question I have is about our own monorepo. When I open the App
or Launchpad
packages in E2E mode then I get a banner to set up Angular. (see screenshot). Is it detecting the Angular in our dependencies from our CT mount adaptor? Should the logic be more specific on what it is searching for monorepos? Should we test this via some system tests to make sure it is giving accurate information with different types of project setups?
"@cypress-design/vue-icon": "0.22.1", | ||
"@cypress-design/vue-statusicon": "0.4.3", |
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.
Why were vue-icon and vue-statusicon updated with this PR? Are these versions that we should have upgraded with the move to Tailwind?
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.
Bumped those as part of troubleshooting the issues with @cypress-design/vue-button
, and now that we're pulling in the latest @cypress-design/css
I figured we'd want to keep the updates in case there were cross-package compatibility issues
* Encapsulate status determination inside `PromptManager` * Use existing `isCTConfigured` flag to standardize detection logic * Fix incorrect icon rotation in Alert header
@warrensplayer Made a small update to use an existing |
Looks like that fixed it for me as well. |
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.
Good catch @warrensplayer - we definitely don't want to recommending configuring CT if it is already configured.
The rest of this looks good to me!
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.
All of my concerns were addressed
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.
Whoops I just noticed a comment I had in draft. I haven't gone through the code in detail but based on the other reviews I don't see a need to, so just a comment about the button size.
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
Note: The majority of files touched in this PR are just renaming the
loginConnectStore
touserProjectStateStore
Creating a new banner to help inform e2e users about component testing. This new banner will display under the following conditions:
component
key in config)Steps to test
ct-banner
). Add dependencies that would satisfy at least one of our auto-detected frameworks - for example,next
orreact
+webpack
.state.json
file (on Mac this will live at~/Library/Application\ Support/Cypress/cy/production/projects/{project}/state.json
, or useDeveloper Tools > View App Data
menu item from launchpad). It should look like this:firstOpened
timestamp to a value >= 4 days ago (easy way is to delete the first digit) and savestate.json
file should now look like this - adjust thedismissed
timestamp to a value >= 2 days agostate.json
should now look like this:state.json
to removedismissed
key fromct_052023_available
banner. Reopen projectdismissed
key and reopening you can validate the various conditions outlined above for when the banner should display/not display (CT not already configured, matching framework detected, etc)How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?