-
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: IATR relevant run status banner #25282
feat: IATR relevant run status banner #25282
Conversation
* Add new splash content for first run pending * Add new build status bar for newer relevant run * Refactor common components for reuse
Thanks for taking the time to open a PR!
|
// TODO GH#24440 Re-map to use relevant run data point (currently stubbed with current run) | ||
const newerRelevantRun = computed(() => run.value) | ||
|
||
const isFirstPendingRun = computed(() => run.value && run.value.runNumber === 1 && run.value.status === 'RUNNING') |
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.
Didn't think this through all the way (run numbers increment across all branches, so this will only work for the very first run on any branch for a given project). I think instead I'll need to grab the count of relevant runs from GQL and see if there's only one relevant run that happens to be pending - that'll build off what Stokes is building now in this branch
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 working to encapsulate the logic in the server side to determine a current
run (what the main part of the Debug page would show) and newer
run (that could be RUNNING
or a completed status). Then the logic here for isFirstPendingRun
would just be currentRun.status === 'RUNNING'
or something like that. And the newer
run would drive the newerRelevantRun
variable
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.
@warrensplayer Sounds good, thanks for the context. Will leave this stubbed for now to ensure visibility & test coverage. If your backend additions merge first I'm happy to hook up as part of this PR, otherwise will plan to handle the hookup as follow-on changes
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.
Looks good - I left some minor comments
> | ||
<div class="flex flex-col w-full items-center"> | ||
<IconTechnologyDashboardRunning class="mb-16px" /> | ||
<span |
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.
Nothing to change here I don't think, but I'm wondering what people's thoughts are on using test IDs for things like this. I usually try to stay away from using data-cy
if I can, since users don't see test IDs on screen.
Is there a reason why we can't just grab the title here with cy.contains
or something like that, since that's what the user would actually see on the page? I understand this makes things a little more brittle (for instance if we decide to update the text then the test will fail). Just asking because I'm trying to refine my own testing strategy.
* Convert "View run" link to use button * Add more top spacing * Fix bg color
Haven't looked at the code yet. Some of the Percy snapshots look a little unexpected to me, but it's unclear if those are related to this PR or the feature branch, since Percy is diffing against develop. I'll look into getting a proper baseline from the actual target branch so that Percy can figure out how to show the impact of this PR on the feature branch, not just develop. |
@marktnoonan @warrensplayer and I we're looking into this for a little bit... I wasn't able to figure out why Percy still compares to the last build on develop even if the PR is targeting a different branch that also has a Percy build. So every Percy run on these IATR PR branches has included every single change from the feature branch as well as the ones from the PR branch |
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.
The refactor to use smaller components is great! The only major thing to address is the source of the data for the counts. The fields you reference in the GQL queries represent "test" counts. For the DebugPendingRunCounts
, it should be counts of "specs" instead. The GQL fields from the Cloud do not currently provide the summary counts, so you have to group the values yourself. Comments in the code give a bit more detail.
// TODO GH#24440 Re-map to use relevant run data point (currently stubbed with current run) | ||
const newerRelevantRun = computed(() => run.value) | ||
|
||
const isFirstPendingRun = computed(() => run.value && run.value.runNumber === 1 && run.value.status === 'RUNNING') |
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 working to encapsulate the logic in the server side to determine a current
run (what the main part of the Debug page would show) and newer
run (that could be RUNNING
or a completed status). Then the logic here for isFirstPendingRun
would just be currentRun.status === 'RUNNING'
or something like that. And the newer
run would drive the newerRelevantRun
variable
* Fix spec vs test total usages
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.
Looks good. We can just keep the mocked out parts for now. I will file a new ticket to resolve those after my merge. Just going to see if we can get Percy straightened out first before merging
Closes #24853
User facing changelog
No changelog entry, merging to feature branch
Additional details
Splash Content
When no previous run exists and current run is "running" then provide a splash page with icon and progress (x of y complete)
Relevant Run banner
Shows new indigo banner below header when a newer run is detected in running or completed status
Items of Note
Stokes is working on the polling logic for this, so I've stubbed out the "newer" run with the current run. This causes some illogical UI states in the screenshots (same run number shows in both current and new run sections)
Stokes is working on the store logic to handle what run to view, stubbed pending that logic being available to integrate
PR is open to add new "running" icon for splash content. Until that is available I've used the "complete" version of the iconUpdatedSteps to test
Data is currently stubbed, best approach to test is to review new tests & percy snapshots
How has the user experience changed?
First run pending
Newer relevant run found
Newer run is in progress
Newer run is complete
PR Tasks
cypress-documentation
?type definitions
?