feat: Display Cypress Dashboard metrics in the Specs Explorer #21250
Conversation
|
Thanks for taking the time to open a PR!
|
|
|
MuazOthman
changed the title
|
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 |
||||||||||||||||||||||||||
MuazOthman
force-pushed
the
muaz/CLOUD-577-spec-list-display-latest-runs
branch
from
4566bee
to
3417dfe
Compare
MuazOthman
changed the base branch from
10.0-release
to
feat/app-cloud-intergation
…UD-577-spec-list-display-latest-runs
…UD-577-spec-list-display-latest-runs
…st-display-latest-runs
MuazOthman
force-pushed
the
muaz/CLOUD-577-spec-list-display-latest-runs
branch
from
72c20df
to
029f489
Compare
* 10.0-release: (22 commits) fix: migrate multiples projects when in global mode (#21458) test: fix flaky cy-in-cy selector validity test (#21360) chore: remove unused codeGenGlobs (#21438) fix: use correct path for scaffolding spec on CT (#21411) fix: remove breaking options from testing type on migration (#21437) fix: test-recording instructions in Component Test mode (#21422) feat: distinguish app vs launchpad utm_source when using utm params (#21424) chore: update stubbed cloud types (#21451) chore: change to yarn registry fix(sessions): refactor flows, fix grouping bugs and align validation fail text (#21379) chore(sessions): more driver tests (#21378) chore: rename domain_fn to origin_fn (#21413) chore: release 9.6.1 (#21404) fix: ensure that proxy logs are updated after the xhr has actually completed (#21373) chore: Re-organize tests in assertions_spec.js (#21283) chore: Distribute tests to desktop-gui containers. Make `desktop-gui` tests faster! (#21305) chore(sessions): add additional tests (#21338) fix: Allow submit button to be outside of the form for implicit submission (#21279) fix(launcher): support Firefox as a snap (#21328) chore(sessions): break out sessions manager code and add unit tests (#21268) ...
packages/frontend-shared/cypress/support/mock-graphql/fakeCloudSpecRun.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I've added some comments, mostly on frontend stuff. I haven't reviewed the tests much yet, and didn't get into detail on the data-context and graphql changes (which I believe have been reviewed as PRs of their own going into this branch).
There are a few things that I'd consider blockers to merging this to develop, that we should discuss:
- The diff for this PR is 5,000 lines and the PR description is 6 words - it should document the changes and rationale for what has been done, explain how to test, the works. It's a big change and lots of head scratching is in our future if we merge without a real description. It will be easier for reviewers who have less context than me to review if there are the usual details and screenshots etc. Even for me, having been in it plenty, I could use a reference point that documents the behavior I should expect. I think we have some hidden rules like, showing runs from the default branch if the feature branch doesn't have runs yet and things like that.
- There are a few existing open questions that don't seem to have been answered, I've tagged people in the threads to get responses, let's get them answered and either complete the TODOs or make the issues for them before we lose context.
- The main technical thing that might be blocker is the fallback to an empty string on this line. An empty string doesn't seem to work downstream, so if there's a chance that the
idon acloudSpecmight really be missing, we'd need to handle it differently. This seems really minor though.
To summarize my other comments:
- There are some places we can remove reactivity that's not needed - computed values and prop bindings that don't use dynamic data.
- We might need to add cleanup for some directory-expansion watchers in the specs list, and we can definitely create fewer watchers be being more selective about when they are needed.
- There are some opportunities to reduce the amount of code and write more idiomatic Vue
- A small amount of deleted code seems to have re-entered the chat after a merge conflict was resolved.
I hit two minor visual bugs. One is as you move your mouse to the login button, if you happen to pass over a table header, it can get stuck open and sit on top of the modal backdrop:
The other is, at the smallest breakpoint here, the vertical alignment inside the rows gets wrong. The "New Spec" button doesn't wrap nicely either, though that's not a part of this PR:
In terms of accessibility, we have some known issues to address and I found a couple of others that I point out in the comments. I'd like to create github issues for these problems and put TODOs pointing to them in the code so we can get things prioritized as quickly as possible after release. A few of the issues are solvable with small HTML changes and I'd suggest we either do those in this PR or, after it is merged, make a follow along PR into develop with the tweaks and the issue numbers for the future TODOs.
My next steps on Monday will be to review the test code itself, run the tests locally, and do more manual testing. A "how to test" section in the PR would be helpful for this so we can hit some kind of matrix of scenarios.
| @@ -112,19 +112,19 @@ describe('App: Spec List (E2E)', () => { | |||
|
|
|||
| describe('typing the filter', function () { | |||
| it('displays only matching spec', function () { | |||
| cy.get('button').contains('14 Matches') | |||
| cy.get('button').contains('23 Matches') | |||
There was a problem hiding this comment.
It's for another day, but every time we touch these tests we have to update a lot of numbers, I wonder if we can either derive them from from our own fixtures, or just move them into constants.
There was a problem hiding this comment.
I think we'd add a task that fetches the correct number, if we do want to address this one in the future.
| @@ -12,7 +12,8 @@ describe('specChange subscription', () => { | |||
| describe('specs list', () => { | |||
| it('responds to specChange event for an added file', () => { | |||
| cy.get('[data-cy="spec-item-link"]') | |||
| .should('have.length', 14) | |||
| // cannot assert a length since this is a virtualized list | |||
There was a problem hiding this comment.
Possibly we can assert "number of matches" here, to get a before/after.
There was a problem hiding this comment.
Why does a virtualized list prevent asserting how many nodes are rendered?
There was a problem hiding this comment.
It doesn't, but it makes it a bit more complex, since the number of nodes rendered is determined by the height of the list in the current conte plus the overscan number that is configured for the list in the Vue component that renders it. It will sometimes be less than then number of matching results for a given search term.
| @@ -1,28 +1,70 @@ | |||
| <template> | |||
| <Tooltip | |||
| <component | |||
There was a problem hiding this comment.
This component is a bit overloaded in the attempt to do two very different things. I suggest we separate the standard idea of a tooltip as a "supplemental label", from the idea of the rich, interactive popups that are tooltip-like in their visual appearance. I'd like to ad a TODO here to come back and create a separate popup version of the heavier tooltip, that would be visually identical but semantically more appropriate to the need, and would support easily entering and exiting the interactive content with the keyboard.
These open up instead of down, but are otherwise closer in spirit to something like the version dropdown in the header than to a tooltip.
There was a problem hiding this comment.
I'd agree with Mark - looks like this is two components, masquerading as one.
Maybe an issue with what you'd like to see. So far, many of our TODOs have remained TODOs for now - I'd also like to get a handle on the frontend which is starting to get a bit messy.
| @@ -107,13 +108,23 @@ import type { LoginModalFragment } from '../../generated/graphql' | |||
|
|
|||
| const online = useOnline() | |||
|
|
|||
| function continueAuth (isLoggedIn: boolean) { | |||
| if (isLoggedIn) { | |||
| emit('loggedin') | |||
There was a problem hiding this comment.
We might as well return early here, since this event triggers a page reload? Hiding the modal will make it seem like we're interactive right away, when really we're refreshing.
There was a problem hiding this comment.
Seems like this had other side-effects actually, reverting
* Cleanup/consolidate Vue watchers * Only watch non-hidden, non-leaf nodes for tree expansion cache * Restructure complex ternary statements * Remove unnecessary reactivity from generated urls * Replace non-breaking space with CSS spacing * Cleanup Vue syntax in prop & event bindings
* Rework tree expansion cache to remove use of `watch`, fix issue with inline usage * Rename poorly-named variable * Reintroduce removed test content
|
|
||
| function simulateRunData () { | ||
| cy.remoteGraphQLIntercept(async (obj) => { | ||
| const fakeRuns = (statuses: string[], idPrefix: string) => { |
There was a problem hiding this comment.
Is there a generated type we can import from somewhere to ensure the shape of this mock stays up to date, even if we add/remove fields?
There was a problem hiding this comment.
Yeah this should probably be defined elsewhere
| } | ||
|
|
||
| function simulateRunData () { | ||
| cy.remoteGraphQLIntercept(async (obj) => { |
There was a problem hiding this comment.
I think we probably want to return this, so we can chain off it like you'd do with any other cy command, should we need to in the future. Otherwise, looking at the usage, it's just
simulateRunData()It's not clear this is actually cy command.
| cy.findByTestId('latest-runs-header').trigger('mouseleave') | ||
|
|
||
| cy.findByTestId('average-duration-header').trigger('mouseenter') | ||
| cy.contains('.v-popper__popper--shown', 'The average spec durations of your latest runs in the Cypress Dashboard') |
There was a problem hiding this comment.
In general, I'm not sure we should assert against a class like .v-popper__popper--shown which is really an implementation detail.
It feels like something along the lines of:
cy.get('h2').contains('The average spec durations of your latest runs in the Cypress Dashboard')or
// note: you'd need to add this selector.
cy.get("[data-cy=modal]").should('be.visible')Are more in line with what a user would do (observe some text exists, observe a DOM element is visible - just because a DOM element exists with some text, doesn't mean it's actually rendered in the viewport, or that a user can see it).
Also, if we moved to another modal library, the test would fail. This would mean we are coupled to the implementation, not the behavior, which isn't ideal.
packages/frontend-shared/src/gql-components/HeaderBarContent.vue
Outdated
Show resolved
Hide resolved
It was something that @brian-mann had wanted in original designs, the ability to show recent runs for a project in global mode. This was mostly just illustrating it being possible, I clarified the |
* Restructure `SpecHeaderCloudDataTooltip` to encapsulate i18n keys * Remove use of 'ACI' in test descriptions * Improve test structure to make existence checks clearer * Simplify function wrapper behavior for tree expansion cache
marktnoonan
dismissed
their stale review
My original change request was about UTM params and has been resolved, and other outstanding requests are being channelled to fast-follows
There was a problem hiding this comment.
All the major blockers are addressed. Approving, with the following in mind:
- Fast follows are reviewed and followed up, epic link. There's definitely some technical debt to be audited and addressed.
- I did not battle test this personally, this was done and owned by ACI team. I focused on the code review primarily.
✔️
mike-plummer
changed the title
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
User facing changelog
Additional details
A large portion of the changes in this PR are to build a GQL mechanism for retrieving & caching Cloud Project data in the app. Whenever possible, previously-cached data is queried and reused by components. As polling proceeds and detects items that may be stale, those individual items can be marked as eligible for re-fetch; once a fetch is initiated they report a status of actively fetching to support display of loading indicators, and finally record a "fetched" or "error" status that can be reflected in the UI.
How has the user experience changed?
Incorporate metrics from the Cypress Dashboard (cloud) into the app in the form of "latest runs" statistics and "average duration" measurements on a per-spec basis. The intent is to increase the value of logging into the dashboard from the app and expose potential benefits of recording test runs that users may not be aware of. Two new columns are being added to the Specs Explorer as shown in the screenshots below.
Logged-out State
For users who are logged out the columns will display disabled placeholders and be otherwise non-interactive.
Activation
Mousing over the headers will display tooltips describing the action needed to activate these columns. Clicking the login button will start the dashboard login/signup flow and, when complete, will return the user.
Active State
Once logged-in the Specs Explorer will display the latest statistics (if available) from the cloud. Each column will display a quickly-scannable overview of its measurement to indicate potential problem areas that may need improvement.
Each column header displays an explanation of the data being displayed and provides a link to online docs for each.
Near-realtime Updates
The app will poll for updates to cloud data for visible specs (within the virtualized list or it's overflow rows) - this polling defaults to every 30 seconds but can be remotely accelerated/decelerated by the dashboard as needed to mitigate any load issues. A single request is made to check if the active project has a recording update within the last 30 seconds - if so, queries are launched for each spec to get the latest data for each. This data is reflected in the app by dynamically-updating row components.
In this screenshot, a recording is currently active - some specs at the top have already Passed, one spec is Running, and the remainder are Queued to be run.
Latest Runs Details
The row items in the "Latest Runs" column gain a mouseover tooltip that exposes additional details on the most recent runs for the given spec.
Not-yet-connected state
If the user is logged in but the active project has not been connected to the dashboard (has no
projectIdin its config file) the user will be given an actionable header tooltip to connect the project.Disconnected State
If the user is logged in but the active project has an invalid
projectIdthe user will be given an actionable header tooltip to re-connect the project.Unauthorized State
If the user is logged in and the project is connected to the dashboard but does not have access to the given
projectIdthey will be given an action to request access. This will then update to show "request sent" once actioned.Offline State
If a loss of network connectivity is detected we now show a warning banner at the top of the Spec Explorer to notify the user.
Steps to test
PR Tasks
cypress-documentation? UDX-218, UDX-207: Document rest of Cypress App cypress-documentation#4554type definitions?cypress.schema.json?Pre-merge tasks: