-
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: Cypress Cloud data on Specs page and Runs page use local Git data if available #26991
Conversation
5 flaky tests on run #47774 ↗︎
Details:
e2e/origin/commands/navigation.cy.ts • 1 flaky test • 5x-driver-chrome:beta
commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-chrome:beta
cypress/cypress.cy.js • 3 flaky tests • 5x-driver-chrome:beta
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
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 going through this and testing it.
I'm posting my QA comments here: #26693
I wonder if I need to test with a built binary, and not the monorepo locally? Could that be causing the issues I'm seeing during my QA?
import { gql, useSubscription } from '@urql/vue' | ||
import { computed } from 'vue' | ||
import { SpecsListRunWatcherDocument } from '../generated/graphql' | ||
import type { RelevantRunInfo } from '@packages/frontend-shared/cypress/support/generated/test-graphql-types.gen' |
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.
Not sure this import is correct - it is importing code from the test package into a production component.
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.
Updated 029679a
(#26991)
Hmm, should this have any effect on the Debug page, or would that be a different issue. I can see a run on the Runs page, and the result of the run on the Specs List, but the Debug page doesn't know about that run yet. Here's a loom, I would start at around 1m 30s to see this: https://www.loom.com/share/5d2a183eadf24fb8b41a7d8afd53fe8e |
When we talk about this using the local git tree, what should my expectations be here, while I am on the On specs and runs page, it shows runs from a branch that I am not currently on called Update - based on #26693 (comment) my expectation wasn't correct, this seems to be working normally. I will read everything once more in the morning, I think I am missing something about how these pages "use local Git data to determine which runs to show" 🤔 |
I think I found the issue I was having in testing @warrensplayer. It was not picking up runs associated with a GH Action triggered via creating a PR, like the ones in red rectangles in this image. I think this is because GH makes a merge commit in the background, which is not present in my local git repo, only the remote, so those were not picked up. I got it working by adding those branches to my This might be an issue, depending on how people have set up their GH Actions workflows. Your logic works as it is supposed to once I enabled running GH actions on my individual branches. This change might cause issues for some users, hard to say, but I'll continue with testing/reviewing and disregard the case of remote only commits for now. I guess this is a fundamental difference between how Cypress Cloud and App works: App uses the local git, and Cloud doesn't care about that at all. I can see this might be a bit confusing for some users, if certain runs appear on Cloud but not locally (sure was confusing for me). |
Update... Stokes pointed out we can work around ^^^ issue using a GH setting: https://github.com/actions/checkout#checkout-pull-request-head-commit-instead-of-merge-commit Good to know for now, this is probably okay for now, we can figure the details as a fast follow. |
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.
This is looking pretty good. I don't see any blocking issues right now. I'll continue testing, but other than the known edge case, which we discussed (GH Actions HEAD commit) the feature seems to be working as expected. The code is far cleaner now too, among the GraphQL patterns getting more streamlined and the improvements to the E2E tests. Great job!
@@ -135,6 +136,24 @@ export function createCloudProject (config: Partial<ConfigFor<CloudProject>>) { | |||
nodes: connectionData.edges.map((e) => e.node), | |||
} | |||
}, | |||
runsByCommitShas (args: CloudProjectRunsByCommitShasArgs) { | |||
console.log('***** runsByCommitShas', args) |
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.
Do we want to remove this log?
runsByCommitShas (args: CloudProjectRunsByCommitShasArgs) { | ||
console.log('***** runsByCommitShas', args) | ||
|
||
return args.commitShas?.map((sha, i) => { |
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.
Nice, this test data is easy to read. Some of the stubs are a bit confusing, I like this style, though. 👍
t.field('relevantRuns', { | ||
type: RelevantRun, | ||
description: 'Subscription that polls the cloud for new relevant runs that match local git commit hashes', | ||
args: { | ||
location: nonNull(enumType({ | ||
name: 'RelevantRunLocationEnum', | ||
members: ['DEBUG', 'SIDEBAR'], | ||
members: ['DEBUG', 'SIDEBAR', 'RUNS', 'SPECS'], |
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.
Should we make this into a separate enum, then re-use it in https://github.com/cypress-io/cypress/pull/26991/files#diff-1cfff3b283c5a3eb1e104bcd05900902c53efbc4ea7286747d8c504adc6cc978R51 ?
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.
Actually we have it, RelevantRunLocationEnum
, we could use on the front-end, too?
fragment RunsContainer_RunsConnection on CloudRunConnection { | ||
nodes { | ||
id | ||
...RunCard |
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.
Not a blocker but relevant to the continued development of GraphQL patterns - we've got two composables tied to the RunCard
fragment, which is defined in UI component. My first instinct was it's not good to couple the data fetching layer to the component, but I'm starting to think this is actually the right pattern - the way we use GraphQL, the fragment is tied to the UI component, so it might actually make sense here.
It's definitely a bit counter intuitive at first, you really need to rethinking a lot of what you'd do with a traditional REST API when working with GraphQL.
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 taking a liking to the pattern when components are self contained and define/manage their own data, especially with the subscriptions and useSubscription composable.
emits('runUpdate') | ||
} | ||
|
||
useSubscription({ query: SpecsListRunWatcherDocument, variables, pause: shouldPause }, handleUpdate) |
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.
Did not realize useSubscription
has a second arg - looks quite useful for transforming data, but also neat usage here to handle an update with the date.
@@ -220,11 +219,6 @@ export class DataContext { | |||
return new ProjectDataSource(this) | |||
} | |||
|
|||
@cached |
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 bye remotePolling ⚰️
Seems our polling / subscriptions story is getting more well defined, any need to update any docs anywhere? We might want to do an update of https://github.com/cypress-io/cypress/blob/develop/guides/graphql-subscriptions.md at some point to capture the ins-and-outs of the whole polling situation, it's not really documented and not very standard - I think it'd be a bit challenging to onboard someone, even with GraphQL experience, to our current App <> Cloud setup.
Not going to block here, but might be a nice thing to do if we have some free time between now and Q3 getting under way.
@marktnoonan No, it should not have any affect on the Debug page. I did note one scenario on that page when testing that I need to retest and write up regarding changing branches. In that case, the Specs and Runs page were seeing the new data for the branch that was switched to right away, but the Debug page was waiting for the next polling cycle (up to 30 sec) to see it. In this case, I watched your Look and I wonder if it has something to do with that run that died that still appears to be running. I will try and recreate the scenario to test. |
@@ -0,0 +1,50 @@ | |||
/** | |||
Non rendering component used to watch running runs and emit an event |
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'm fine with a non-rendering component, but is this something that might as well be a composable that exposes a ref, instead of a component that emits an event?
I have definitely made at least one renderless component mostly to follow the convention we had of doing GQL requests in SFCs, but we seem to have not maintained that convention in other place. So just curious if there's some other reason to make this a component that I'm missing? (Idk, ease of hooking into lifecycle or something?).
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 was not sure of another way of using the Urql useSubscription
across an array of options without having this renderless component. This implementation allows it to get mounted for each run on the page after the original page has mounted and the GQL queries returned. If anyone has another pattern for this, please share!
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.
This seems fine to me.
cy.findByTestId('sidebar-link-debug-page').click() | ||
cy.findByTestId('debug-container').should('be.visible') | ||
|
||
cy.findByTestId('header-top').contains('chore: testing cypress') |
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.
In general I would like to add some more visibility assertions to this spec file where we use contains
, particularly because we do not have any percy snapshots in app e2e tests.
Without visibility assertions, we are confirming what is in the DOM, but we won't catch problems caused by CSS that might still hide the elements in various ways.
For example, for this assertion, we are passing at this point, after DOM is updated but before CSS rendering has caught up:
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.
makes me really want to make a containsVisible
custom command and just never think about this again.
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.
Well done making testing better, I sure to love to see those deletions. Local testing all checks out, I poked around in the various test cases as well and all seems good to me.
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
Before this change, the Specs page and the Runs page did not use local Git data to determine which runs to show.
Specs page
For the Specs page, an algorithm was used within the Cloud query to find runs for the current Branch name and then fall back to the
default
branch in the Cloud before that. This did not always match the actual Git tree the developer was using. The new logic will use the same polling introduced with the Debug page to pull the "latest" runs that match the Git tree and pass those to new Cloud GQL fields to determine spec statuses for those runs. The code changes underpackages/app/src/specs
will reflect the update to the GQL fields used. The updates topackages/app/src/pages/Specs/Index.vue
include the introduction of theuseRelevantRuns
composable.Runs page
For the Runs page, it previously only just pulled the latest 10 runs from the Cloud project. It would cache these runs and then use a cursor pattern to pull new runs that were more recent than the latest run. Now, the runs page preserves that logic as a fallback and it has been encapsulated into the
useProjectRuns
composable. Another composable calleduseGitTreeRuns
was added that uses theuseRelevantRuns
composable to pull runs relevant to the local Git tree and use those runs to display. This will always just show the latest 10 runs for the local Git tree. This may be expanded in a future initiative.For either scenario,
RunCard
now uses a subscription to update itself if it is currently Running.Additional changes on this branch
Steps to test
Testing with Git
Testing without Git
.git
folderHow has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?