-
Notifications
You must be signed in to change notification settings - Fork 25
DEVPROD-4166 Add breaking commits option in previous commits dropdown #2254
DEVPROD-4166 Add breaking commits option in previous commits dropdown #2254
Conversation
5 flaky tests on run #15969 ↗︎
Details:
Review all test suite changes for PR #2254 ↗︎ |
@ZackarySantana is this pr open for review? You tagged the team but the title still says [NOT FOR REVIEW] |
Yes! It is, sorry about that |
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.
looking good so far!
|
||
describe("previous commits", () => { | ||
describe("relevant commits", () => { |
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.
Is it possible to add a test in here for the new "breaking commit" button? :0
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 added to the existing tests instead of a new one and added some data! I think it makes sense to test it like the others but I am new to testing and if you feel otherwise I can switch it up
@@ -42,6 +46,7 @@ export const PreviousCommits: React.FC<PreviousCommitsProps> = ({ taskId }) => { | |||
buildVariant, | |||
displayName, | |||
projectIdentifier, | |||
status: baseStatus, |
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 think you may still want to call this status
because it's the status of the current task being viewed (rather than its base task status)!
status: baseStatus, | |
status, |
baseStatus === TaskStatus.Succeeded, | ||
variables: { | ||
projectIdentifier, | ||
skipOrderNumber: passingOrderNumber + 2, |
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.
Is the +2 being used to work around the $lt
operator that's in use here? I think a comment might be helpful since +2 is a bit unusual
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.
Yes it is! I can add a comment
skip: | ||
!parentTask || | ||
!lastPassingTask || | ||
baseStatus === undefined || |
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.
Curious if this is needed! I feel like the task status should not be undefined
but let me know if you encountered a scenario where it was 👀
baseStatus === undefined || |
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.
![Screenshot 2024-02-20 at 4 08 54 PM](https://private-user-images.githubusercontent.com/64446617/306416655-673bbbd2-b140-4741-adcf-5c593d2b7451.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzQ1MDczNTAsIm5iZiI6MTczNDUwNzA1MCwicGF0aCI6Ii82NDQ0NjYxNy8zMDY0MTY2NTUtNjczYmJiZDItYjE0MC00NzQxLWFkY2YtNWM1OTNkMmI3NDUxLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDEyMTglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQxMjE4VDA3MzA1MFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTVmNGJmMTZmOGU2YmIyMDZmMGMyNDI1ZjRkMWI1YzhhYzU4YzAzYThhYjA1ZWM4NjZhZmY2MTNmODJmOWE3MzMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.vn0UF726vf1vQ0dohSLVL7mPWWZpGdwBlFAfUdFpoNY)
It starts as undefined and then updates - but that's because the baseTask starts as undefined and an empty object is used.
I did some more testing and I think it is needed even though the other conditions usually cause this query to skip too but I'm only worried about a race condition- i.e. parentTask and lastPassingTask load before the current task.
EDIT: Screenshot is a useEffect with only status, I did some more testing with the other skip variables that I didn't show since it's more confusing
@@ -22,6 +22,7 @@ query LastMainlineCommit( | |||
} | |||
} | |||
id | |||
order |
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.
You can get the order
from a task as well (in case that makes passingOrderNumber
calculation easier i.e. lastPassingTask.order
). Just something I wanted to call out!
a1d62fb
to
c1d95e0
Compare
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.
sorry it took me some time to get back to you!
status === undefined || | ||
status === TaskStatus.Succeeded, |
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.
Got it — In that case, I wonder if you instead want to check that the status isn't a failing task status! I think that you'll want to use the function isFailedTaskStatus
, which you can import at the top of the file like:
const { isFailedTaskStatus, isFinishedTaskStatus } = statuses;
I think this should eliminate the need to specifically check for undefined
! 🙂
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.
Hm, I don't think this would eliminate it since we should check for !isFailedTaskStatus
which undefined
would return true
, then the !
would make it false
and not skip for undefined status.
I think I could keep the undefined check or switch it to do status ?? TaskSucceeded
(not sure how to reference task success status but however it is)- but that may be confusing. What do you think?
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 feel like isFailedTaskStatus(status)
should return false
for an undefined
status, and then the !isFailedTaskStatus(status)
should make it true
to skip. Is that not the case for the function? 👀
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.
You are right, not sure what I was thinking when I wrote the above... Thank you for being thorough!
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.
You are right, not sure what I was thinking when I wrote the above... Thank you for being thorough!
sendEvent({ | ||
name: "Submit Previous Commit Selector", | ||
type: CommitType.Breaking, | ||
}) |
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 wonder if we want to rename the analytics event as well. It would cause us to "reset" analytics on this, but it's not something we query often anyways. (Personally I think it's okay to rename 👀)
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 would be down to switch it! If it helps properly track analytics for it, but I'm not sure of the process or what they should look like. I was just trying to emulate the other event sendings. I think if it lets us tally and filter by the type
field I think we could keep it the same
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.
discussed offline; will change the analytics name to Relevant
rather than Previous
|
||
// The breaking commit is the first failing commit after the last passing commit. | ||
// The skip order number should be the last passing commit's order number + 1. | ||
// We use + 2 because internally the query does a less than comparison. |
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 would suggest to leave a permalink here to the GitHub line:
We use + 2 to work around the underlying query which uses the $lt operator (link here).
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 added a link but please let me know if I did it right! I used the HEAD commit currently of the current branch so it's unchanging in the future.
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.
discussed offline; will change link to link to the query in the Evergreen repo
@@ -156,6 +185,19 @@ export const PreviousCommits: React.FC<PreviousCommitsProps> = ({ taskId }) => { | |||
> | |||
Go to {versionMetadata?.isPatch ? "base" : "previous"} commit | |||
</MenuItem> | |||
<MenuItem | |||
as={Link} | |||
disabled={breakingLoading || breakingTask === undefined} |
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 more of a UX comment, but when testing this on larger projects (like mongodb), it can take a while for the query to finish. I think it could be good to have some indication that the query is still loading (maybe a loading spinner)?
You could make a separate ticket to explore this, since it wasn't covered by the original designs! Or you can experiment (but only if you want to)!
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.
Hm that might be a good idea. I think adding a loading indicator to the relevant task button might be useful but from my testing, usually it loads as fast as the main page data. I think it would be nice if we did think of some standard for links/buttons/sections that are in the process of loading but we have sent a query for them.
I thought it could be a less grayed out button, italicized, some small icon next to it, or a loading spinner next to it. I just tried some with like italicized but I'm not confident in how they look so I think I'll leave it up for future discussion
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.
lgtm mod applying the changes that we discussed offline! great work!
|
||
// The breaking commit is the first failing commit after the last passing commit. | ||
// The skip order number should be the last passing commit's order number + 1. | ||
// We use + 2 because internally the query does a less than comparison. |
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.
discussed offline; will change link to link to the query in the Evergreen repo
sendEvent({ | ||
name: "Submit Previous Commit Selector", | ||
type: CommitType.Breaking, | ||
}) |
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.
discussed offline; will change the analytics name to Relevant
rather than Previous
I am not used to these mocked queries, I just finally understood them and got a working/sensible test case for the breaking commit selector! |
DEVPROD-4166
Description
Designs
Note that the designs include a guide cue and task metadata addition- but those have been separated to different tickets.
Screenshots
Simple Mainline Commit Example:
https://github.com/evergreen-ci/spruce/assets/64446617/89f7d4c4-004f-4f0a-a0b0-48d93beb4f32
Complex Mainline Commit Example (Stepback Bisection):
https://github.com/evergreen-ci/spruce/assets/64446617/2353e1ba-00f8-4ee6-bbed-73596b4505f9
CLI Example:
https://github.com/evergreen-ci/spruce/assets/64446617/9b6bb2da-a153-4456-a942-0e54457f2a81
Testing
Manual testing with different commits/tasks and appended info to existing tests