chore: show correct cloud project title in notifications#27095
Conversation
29 flaky tests on run #48037 ↗︎
Details:
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| network stubbing > intercepting request > can delay and throttle a StaticResponse |
Output
|
|
cypress/cypress.cy.js • 3 flaky tests • 5x-driver-firefox
| Test | Artifacts | |
|---|---|---|
| ... > correctly returns currentRetry |
Output
|
|
| ... > correctly returns currentRetry |
Output
|
|
| ... > correctly returns currentRetry |
Output
|
|
create-from-component.cy.ts • 1 flaky test • app-e2e
| Test | Artifacts | |
|---|---|---|
| ... > runs generated spec |
Output
Screenshots
Video
|
|
specs_list_latest_runs.cy.ts • 1 flaky test • app-e2e
| Test | Artifacts | |
|---|---|---|
| App/Cloud Integration - Latest runs and Average duration > when no runs are recorded > shows placeholders for all visible specs |
Output
Screenshots
Video
|
|
cypress-in-cypress-component.cy.ts • 1 flaky test • app-e2e
| Test | Artifacts | |
|---|---|---|
| Cypress In Cypress CT > default config > redirects to the specs list with error if a spec is not found |
Output
Screenshots
Video
|
|
The first 5 flaky specs are shown, see all 17 specs in Cypress Cloud.
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.
| this.api.routeToDebug() | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Seems we don't have a centralized place to get info about Cloud projects, so I added this one. Not sure if this is the right pattern - I also think we'd likely want to call this earlier in the code (maybe as soon as we know they are authenticated and they've selected a project?).
Either way, we need to ensure we fetch the cloud project so we can get the name, so I added this function.
| async fetchCloudProjectInfo (): Promise<CoreDataShape['cloud'] | undefined> { | ||
| const projectSlug = await this.ctx.project.projectId() | ||
|
|
||
| if (this.ctx.coreData.cloud.id) { |
There was a problem hiding this comment.
Caching - we only need to fetch this once.
There was a problem hiding this comment.
It does not appear that this caching would ever get cleared. Would it? What about running in Global Mode and switching between projects? Or reconnecting a local project to a different Cypress Cloud project?
There was a problem hiding this comment.
Perhaps clearing out the values in clearCurrentProject above is a start to solve some scenarios. But not sure how to handle the case of someone switching branches that has a different Cloud projectId in the config file or just manually editing the config file.
I would also not want to spread this "reset" logic randomly through the code. I will keep looking for other solutions.
There was a problem hiding this comment.
I thought we clear the entire data-context between projects - this may not be the case, let me find out.
There was a problem hiding this comment.
Changed this, please see the updated code. We still cache, but the data is it a separate key (cloud) and we have a single place to clear it between projects based on the existing pattern of calling lifecycle.clearCurrentProject()
astone123
left a comment
There was a problem hiding this comment.
This worked as expected for me, and I think adding the action to load the cloud data where you did is fine 👍🏻
warrensplayer
left a comment
There was a problem hiding this comment.
Nice job removing that unneeded parameter to executeRemoteGraphQL. That does clean things up a bit.
My main concern is with the caching. (See my other comments).
Secondary concern is with adding GQL stuff to ProjectActions. I cannot think of a better place to put it at the moment and there is a similar pattern in AuthActions.
What do you think about adding a CloudProjectActions / DataSource? |
|
We will discuss in an internal thread. |
|
We decided
|
| this.ctx.actions.electron.showSystemNotification(this.projectTitle, body, () => this.onNotificationClick(runNumber)) | ||
| async #showRunNotification (body: string, runNumber: number) { | ||
| try { | ||
| const cloudProjectMetadata = this.ctx.coreData.cloudProject.metadata ?? await this.ctx.actions.cloudProject.fetchMetadata() |
There was a problem hiding this comment.
I couldn't find a single place that makes sense to call this earlier in the lifecycle, so I'm just calling it when we first show a notification. We cache the data. Said data is cleared when we call clearCloudProject, which is called in the clearCurrentProject() method (where all cleanup between projects occurs).
This does not really feel ideal, but it follows the convention we have (clean up in one place) and at the very least it is simple - you don't need to figure out how / if the cloud metadata is set, it's called right before the data is needed.
My only concern here is this will block showing the notification until the request finishes - that said, we are constantly in sync with the Cloud via the runs polling anyway, so we know the Cloud is alive and responding - worse case scenario is your notification might be delayed by a second or two, which won't be an issue - it's not really real time anyway right now, since we are polling.
Additional details
Show the Cloud Project name in the notification. Previously it was the local project, based on the directory name, which is incorrect.
We did not have a central place to fetch and collect cloud project metadata, so I added one. I also made a small refactor - we were duplicating
operationandoperationDocin many GraphQL remote requests, so I collapsed them to use a single property,operationDoc.operationis derived fromoperationDocviaprintfrom thegraphqlpackage.Steps to test
How has the user experience changed?
Comply with requirements in the initiative: https://app.clickup.com/18033298/docs/h6amj-43107/h6amj-41927?block=block-7032488f-6f28-4593-a428-dbb17f12c0cd
PR Tasks
cypress-documentation?type definitions?