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

feat: Display Cypress Dashboard metrics in the Specs Explorer #21250

Merged
merged 138 commits into from
Jun 27, 2022

Conversation

MuazOthman
Copy link
Contributor

@MuazOthman MuazOthman commented Apr 28, 2022

User facing changelog

  • Add new "Latest Runs" and "Average Duration" columns capable of displaying data from the Cypress Dashboard

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.

Screen Shot 2022-06-26 at 4 42 31 PM

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.

Screen Shot 2022-06-26 at 4 30 38 PM

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.

Screen Shot 2022-06-26 at 4 29 10 PM

Each column header displays an explanation of the data being displayed and provides a link to online docs for each.

Screen Shot 2022-06-26 at 4 29 31 PM

Screen Shot 2022-06-26 at 4 29 39 PM

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.

Screen Shot 2022-06-26 at 4 35 11 PM

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.

Screen Shot 2022-06-26 at 4 29 23 PM

Not-yet-connected state
If the user is logged in but the active project has not been connected to the dashboard (has no projectId in its config file) the user will be given an actionable header tooltip to connect the project.

Screen Shot 2022-06-26 at 4 48 37 PM

Disconnected State
If the user is logged in but the active project has an invalid projectId the user will be given an actionable header tooltip to re-connect the project.

Screen Shot 2022-06-26 at 4 31 47 PM

Unauthorized State
If the user is logged in and the project is connected to the dashboard but does not have access to the given projectId they will be given an action to request access. This will then update to show "request sent" once actioned.

Screen Shot 2022-06-26 at 4 32 40 PM

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.

Screen Shot 2022-06-26 at 4 30 17 PM

Steps to test

  • Confirm all "disconnected" states are correct when logged out
  • Log in via the CTAs in the header tooltips
  • Confirm all project data is shown correctly after login
  • Perform actions such as recording runs, navigation, re-authentication, network throttling, etc.
  • Confirm all states are shown and respond to actions correctly

PR Tasks

Pre-merge tasks:

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Apr 28, 2022

Thanks for taking the time to open a PR!

@CLAassistant
Copy link

CLAassistant commented Apr 28, 2022

CLA assistant check
All committers have signed the CLA.

@MuazOthman MuazOthman changed the title WIP: display latest runs feat: Display latest runs in the spec list Apr 28, 2022
@cypress
Copy link

cypress bot commented Apr 28, 2022



Test summary

4904 0 63 0Flakiness 1


Run details

Project cypress
Status Passed
Commit a874fa8
Started Jun 27, 2022 8:46 PM
Ended Jun 27, 2022 9:00 PM
Duration 13:58 💡
OS Linux Debian - 10.11
Browser Firefox 98

View run in Cypress Dashboard ➡️


Flakiness

cypress/e2e/e2e/origin/commands/assertions.cy.ts Flakiness
1 cy.origin assertions > #consoleProps > .should() and .and()

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 MuazOthman force-pushed the muaz/CLOUD-577-spec-list-display-latest-runs branch from 4566bee to 3417dfe Compare May 3, 2022 18:11
@MuazOthman MuazOthman changed the base branch from 10.0-release to feat/app-cloud-intergation May 3, 2022 20:23
@MuazOthman MuazOthman force-pushed the muaz/CLOUD-577-spec-list-display-latest-runs branch from 72c20df to 029f489 Compare May 13, 2022 00:05
* 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)
  ...
Copy link
Member

@tgriesser tgriesser left a comment

Choose a reason for hiding this comment

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

Just a few comments while looking through things, look forward to playing around with it a bit with the new API

packages/app/src/specs/SpecRunSummary.vue Outdated Show resolved Hide resolved
packages/app/src/runs/RunCard.vue Outdated Show resolved Hide resolved
packages/app/src/specs/SpecRunSummary.cy.tsx Outdated Show resolved Hide resolved
packages/app/src/runs/RunResults.vue Outdated Show resolved Hide resolved
packages/app/src/specs/SpecRunSummary.vue Outdated Show resolved Hide resolved
packages/app/src/specs/SpecHeaderCloudDataTooltip.vue Outdated Show resolved Hide resolved
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.

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 id on a cloudSpec might 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:

Screen Shot 2022-06-25 at 11 38 46 AM

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:

Screen Shot 2022-06-25 at 4 17 24 PM

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')
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly we can assert "number of matches" here, to get a before/after.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does a virtualized list prevent asserting how many nodes are rendered?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

packages/app/src/runs/RunsErrorRenderer.vue Outdated Show resolved Hide resolved
packages/app/src/specs/LastUpdatedHeader.vue Outdated Show resolved Hide resolved
@@ -1,28 +1,70 @@
<template>
<Tooltip
<component
Copy link
Contributor

@marktnoonan marktnoonan Jun 25, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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')
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Will add an early return

Copy link
Member

@tgriesser tgriesser Jun 27, 2022

Choose a reason for hiding this comment

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

Seems like this had other side-effects actually, reverting

packages/app/src/specs/SpecsList.vue Outdated Show resolved Hide resolved
packages/launchpad/src/setup/OpenBrowserList.vue Outdated Show resolved Hide resolved
* 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
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.

I left a bunch of comments/questions. Happy to do a sync/pair code review for more clarity. I did NOT test this out locally yet.


function simulateRunData () {
cy.remoteGraphQLIntercept(async (obj) => {
const fakeRuns = (statuses: string[], idPrefix: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this should probably be defined elsewhere

}

function simulateRunData () {
cy.remoteGraphQLIntercept(async (obj) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

packages/app/cypress/e2e/specs_list_latest_runs.cy.ts Outdated Show resolved Hide resolved
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')
Copy link
Contributor

Choose a reason for hiding this comment

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

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/graphql/urqlClient.ts Outdated Show resolved Hide resolved
packages/server/lib/makeDataContext.ts Show resolved Hide resolved
packages/server/lib/makeDataContext.ts Outdated Show resolved Hide resolved
@tgriesser
Copy link
Member

This TODO doesn't make sense, we already tried this on one 544 - if it's not there, it's not available, unless they are in global mode.

Why do we even need to do this here? Eg - why grab from the savedState at all?

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 todo to call out that we should add it if we ever want to do that.

mike-plummer and others added 3 commits June 27, 2022 12:30
* 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 marktnoonan dismissed their stale review June 27, 2022 20:42

My original change request was about UTM params and has been resolved, and other outstanding requests are being channelled to fast-follows

@lmiller1990 lmiller1990 self-requested a review June 27, 2022 22:20
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.

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 mike-plummer changed the title feat: Display latest runs in the spec list feat: Display Cypress Dashboard metrics in the Specs Explorer Jun 27, 2022
@mike-plummer mike-plummer merged commit 4726ea9 into develop Jun 27, 2022
@mike-plummer mike-plummer deleted the muaz/CLOUD-577-spec-list-display-latest-runs branch June 27, 2022 22:37
tgriesser added a commit that referenced this pull request Jun 28, 2022
…est-runs-batching

* develop:
  fix: add baseUrl to TestConfigOverrides (#22445)
  fix: distribute files to machines for external contributors. (#22326)
  feat: Display Cypress Dashboard metrics in the Specs Explorer (#21250)
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 28, 2022

Released in 10.3.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v10.3.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Jun 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet