DEVPROD-4608 Add stepback to the metadata of tasks #2299

ZackarySantana · 2024-03-12T16:03:13Z

Description

This adds the metadata item for stepback to show "In progress" or "completed" and an info icon accompanying it. I implemented according to the design except I added a "loading" state while the graphql query completes.

This also isolates and extracts the different relevant commit queries in to each their own hook: 'useParentTask', 'useLastPassingTask', 'useBreakingCommit', and 'useLastExecutedTask'. The first three are re-used (within each other, in the relevant commits dropdown, and the metadata card) while the last one is only used in the relevant commits dropdown- but it figured it would be better to extract and test that logic separately to be consistent (it removes bloat from the relevant commits dropdown component).

I separated my commits to be distinct so anyone who reviews can look over the commits for an easier time!

Screenshots

In progress

Completed

Hovering dropdown

Testing

I tested the buttons on multiple different tasks on staging- before and after this change. All the buttons respond the exact same- and as for the metadata item I also tested on staging making sure it shows the correct state.

I plan to add similar testing for all the new hooks if the reviewer thinks the tests for the hook I added is good! Please let me know- it shouldn't be too long to add them!

cypress · 2024-03-12T16:28:24Z

5 flaky tests on run #16261 ↗︎

0	549	7	0	5
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

feat: switch from tsx to ts
Project: Spruce	Commit: `2f988c73b7`
Status: Passed	Duration: 21:04 💡
Started: Mar 19, 2024 6:07 PM	Ended: Mar 19, 2024 6:28 PM

Review all test suite changes for PR #2299 ↗︎

ZackarySantana · 2024-03-19T18:53:18Z

Could anyone on UI help me with the hooks tests? I defined the mocks and I'm using the renderHook with a wrapper that is a provider with the mocks- but it does not get any of the mocks.

Here's a quick view of it but it's available in the useBreakingTask hook and test file.

Test usage part:

   const { result } = renderHook(() => useBreakingTask("t1"), {
      wrapper: ({ children }) =>
        ProviderWrapper({
          children,
          mocks: [
            getPatchTaskWithFailingBaseTask,
            getLastPassingVersion,
            getBreakingCommit,
          ],
        }),
    });

The mock I'm providing:

const getPatchTaskWithFailingBaseTask: ApolloMock<
  BaseVersionAndTaskQuery,
  BaseVersionAndTaskQueryVariables
> = {
  request: {
    query: BASE_VERSION_AND_TASK,
    variables: {
      taskId: "t1",
    },
  },
  result: {
    data: {
      task: {
      ...

The query I'm expecting the mock to fill (but it prints undefined)

  const { data: taskData } = useQuery<
  BaseVersionAndTaskQuery,
  BaseVersionAndTaskQueryVariables
>(BASE_VERSION_AND_TASK, {
  variables: { taskId },
});

const { buildVariant, displayName, projectIdentifier, status } =
  taskData?.task ?? {};
console.log(taskId);
console.log(taskData?.task);

taskId properly puts out t1 but the taskData?.task is undefined.

minnakt

test structure lgtm 😎 feel free to add the rest! and great reorganization with the hooks!

minnakt · 2024-03-20T02:31:01Z

src/hooks/useBreakingTask/useBreakingTask.test.ts

@@ -0,0 +1,180 @@
+import { ProviderWrapper } from "components/HistoryTable/hooks/test-utils";


this ProviderWrapper is specifically for the HistoryTable component. You probably want to define your own ProviderWrapper in this file instead!

minnakt · 2024-03-20T02:35:29Z

src/hooks/useBreakingTask/useBreakingTask.test.ts

+
+    expect(result.current.task).toBeDefined();


I think you should be able to get this to pass if you wrap it in await waitFor(() => { // expect here })

You may also want to add an assertion like expect(result.current.task.id).toBe("breaking_commit"); after this one

minnakt · 2024-03-20T02:49:52Z

src/hooks/useBreakingTask/useBreakingTask.test.ts

+
+    expect(result.current.task).toBeDefined();
+  });
+});


(optional) another test case you may want to test is the error case — would require creating an additional GraphQL mock that returns an error. in this test, you can assert that the error toast is dispatched as expected

minnakt · 2024-03-20T02:50:36Z

src/context/toast/__mocks__/index.tsx

+ * It should only be used in favor of RenderFakeToastContext when the component
+ * requires children to be rendered.
+ */
+const MockToastContext = () => {


From what I can tell, I think you can achieve the same effect in your tests with the existing function RenderFakeToastContext:

const { dispatchToast } = RenderFakeToastContext(); const { result } = renderHook(() => useBreakingTask("t1"), { wrapper: ({ children }) => ProviderWrapper({ children }), });

(see example hook test with toast mocks in Toast.test.tsx)

then you can make assertions in your test that dispatchToast is or is not called :)

I added this for the ones that dispatch a toast notification but excluded it for those that don't!

minnakt · 2024-03-20T03:01:14Z

src/pages/task/metadata/Stepback.tsx

+  // nextStepbackTaskId is set when the next task in stepback in known, in the beginning
+  // of stepback, it is known right away. In the rest of stepback, it is not.


nit. there might be a few typos in this comment e.g. in stepback in known. in the beginning of... seems like it should be its own sentence as well

minnakt · 2024-03-20T03:05:42Z

src/pages/task/metadata/Stepback.tsx

+      <Tooltip
+        align="top"
+        justify="middle"
+        trigger={
+          <IconContainer>
+            <Icon glyph="InfoWithCircle" size="small" />
+          </IconContainer>
+        }
+        triggerEvent="hover"
+      >
+        <Body>
+          When Stepback is completed you can access the breaking commit via the
+          relevant commits dropdown.
+        </Body>
+      </Tooltip>{" "}


You might be able to replace this block with the InfoSprinkle component

minnakt · 2024-03-20T03:27:28Z

src/pages/task/metadata/Stepback.tsx

+          relevant commits dropdown.
+        </Body>
+      </Tooltip>{" "}
+      {loading && <Badge variant="lightgray">Loading</Badge>}


Having Loading in a badge makes it seem like it's a state of stepback, rather than an indication that we're still querying for a response. Perhaps a Skeleton Loader or Spinner could be a good replacement? (a skeleton loader is good for longer-term loading operations > 300ms, a spinner is good for shorter-term loading operations < 300ms)

It's hard to guage how fast this might take. I based it on relevant commits since it uses the same queries, and guessed on average it would be a longer operation so I did the skeleton loader for the badge!

ZackarySantana · 2024-03-20T16:27:47Z

New loading state!:

Using new sprinkle, looks near identical:

…tched

minnakt

added tests look great!

minnakt · 2024-03-20T21:23:12Z

src/hooks/useBreakingTask/useBreakingTask.test.tsx

+);
+
+describe("useBreakingTask", () => {
+  it("no breaking task is found when task is not found", async () => {


nit. You can remove async since this test doesn't await anything (applies to other test files as well)

Nice catch! I wonder why the linter didn't catch it...

minnakt · 2024-03-20T21:31:56Z

src/pages/task/metadata/Stepback.tsx

+ * @param task The task to check if it is in stepback.
+ * @returns Whether the task is in stepback.
+ */
+export function inStepback(task: TaskQuery["task"]) {


nit. I'd maybe rename this function to something like isInStepback to more clearly indicate that it returns a boolean

minnakt · 2024-03-20T21:38:00Z

src/pages/task/metadata/Stepback.tsx

+  const stepback =
+    task?.stepbackInfo?.lastFailingStepbackTaskId !== undefined &&
+    task?.stepbackInfo?.lastFailingStepbackTaskId !== "";


I would suggest a more boolean-y name, and I think you can simplify it to something like this:

Suggested change

const stepback =

task?.stepbackInfo?.lastFailingStepbackTaskId !== undefined &&

task?.stepbackInfo?.lastFailingStepbackTaskId !== "";

const hasLastStepback = task?.stepbackInfo?.lastFailingStepbackTaskId?.length > 0;

minnakt · 2024-03-20T21:38:26Z

src/pages/task/metadata/Stepback.tsx

+  const beginningStepback =
+    task?.stepbackInfo?.nextStepbackTaskId !== undefined &&
+    task?.stepbackInfo?.nextStepbackTaskId !== "";


same comment!

Suggested change

const beginningStepback =

task?.stepbackInfo?.nextStepbackTaskId !== undefined &&

task?.stepbackInfo?.nextStepbackTaskId !== "";

const isBeginningStepback = task?.stepbackInfo?.nextStepbackTaskId?.length > 0;

minnakt · 2024-03-20T21:48:03Z

src/pages/task/metadata/Stepback.tsx

+  // The last stepback task has an undefined last passing task (it is passing itself).
+  const isLastStepbackTask = lastPassingTask === undefined;
+
+  // The stepback is finished if there is a breaking task or we are on the last stepback task.
+  const finished = breakingTask !== undefined || isLastStepbackTask;


Is it necessary to check lastPassingTask? I feel like we always expect there to be a breaking task as long as stepback is finished — if breaking task was not defined then users wouldn't be able to access it via the Relevant Commits dropdown as the tooltip instructs

If you are on the breaking task, stepback should still be counted as finished- the breaking task is when lastPassingTask === undefined is true. I'll redo the comment!

minnakt · 2024-03-20T22:01:11Z

src/pages/task/metadata/Stepback.tsx

+    <StepbackWrapper>
+      Stepback:
+      <InfoSprinkle baseFontSize={BaseFontSize.Body1}>
+        When Stepback is completed you can access the breaking commit via the
+        relevant commits dropdown.
+      </InfoSprinkle>
+      {loading && <Skeleton size="small" />}
+      {!loading && !finished && <Badge variant="lightgray">In progress</Badge>}
+      {!loading && finished && <Badge variant="green">Complete</Badge>}
+    </StepbackWrapper>


My only reservation about this is that now this doesn't have the correct font styles from MetadataItem (if you look closely the font size is wrong). Perhaps we can change the underlying HTML of MetadataItem into a <div> and then rewrite it as the following:

Suggested change

<StepbackWrapper>

Stepback:

<InfoSprinkle baseFontSize={BaseFontSize.Body1}>

When Stepback is completed you can access the breaking commit via the

relevant commits dropdown.

</InfoSprinkle>

{loading && <Skeleton size="small" />}

{!loading && !finished && <Badge variant="lightgray">In progress</Badge>}

{!loading && finished && <Badge variant="green">Complete</Badge>}

</StepbackWrapper>

<MetadataItem>

<StepbackWrapper>

// content goes here

</StepbackWrapper>

</MetadataItem>

minnakt · 2024-03-20T22:24:32Z

src/pages/task/actionButtons/RelevantCommits/index.tsx

-    versionMetadata,
-  } = taskData?.task ?? {};
-  const { order: skipOrderNumber } = versionMetadata?.baseVersion ?? {};
+  const { baseTask, versionMetadata } = taskData?.task ?? {};


I just realized that there doesn't seem to be a reason to perform the BaseVersionAndTaskQuery at all; it looks like the parent component of this component (ActionButtons.tsx) can pass these values down as props 🤔

minnakt · 2024-03-21T15:35:17Z

src/pages/task/ActionButtons.tsx

@@ -254,7 +254,7 @@ export const ActionButtons: React.FC<Props> = ({
      <PageButtonRow>
        {!isExecutionTask && (
          <>
-            <RelevantCommits taskId={taskId} />
+            <RelevantCommits taskId={taskId} task={task} />


You can remove the taskId prop if it can be accessed from the task!

minnakt · 2024-03-21T15:45:24Z

src/components/MetadataCard.tsx

  children,
  "data-cy": dataCy,
-}) => <Item data-cy={dataCy}>{children}</Item>;
+}) => (
+  <Item data-cy={dataCy} as={as}>


This is cool! The only issue with this is that the margin from :not(:last-of-type) selector doesn't quite work as intended anymore due to the mix of <p> and <div> tags.

Maybe we can change :not(:last-of-type) -> :not(:last-child)

minnakt · 2024-03-21T15:45:54Z

src/pages/task/metadata/Stepback.tsx

+};
+
+const StepbackWrapper = styled.div`
+  margin-top: ${size.xxs};


you can remove this margin-top style if you get the margin from the style change mentioned in the previous comment

minnakt · 2024-03-21T15:47:00Z

src/pages/task/metadata/Stepback.tsx

+  const isBeginningStepback =
+    task?.stepbackInfo?.nextStepbackTaskId !== undefined &&
+    task?.stepbackInfo?.nextStepbackTaskId !== "";


Can this be rewritten with length as well, or did you run into a problem? 👀

Nope! Just got confused but I updated it to use the length trick too

minnakt

great work!

minnakt · 2024-03-21T16:45:33Z

src/pages/task/metadata/index.tsx

+            data-cy="t
+      task: patchTaskWithSuccessfulBaseTask,
+    ask-metrics-link"


looks like this was an unintentional change

minnakt · 2024-03-21T16:52:53Z

src/pages/task/metadata/Stepback.tsx

+          When Stepback is completed you can access the breaking commit via the
+          relevant commits dropdown.
+        </InfoSprinkle>
+        {loading && <Skeleton size="small" />}


nit. loading state looks like it might need a small style tweak

Added no wrap and it looks like this now!

ZackarySantana added 3 commits March 12, 2024 11:52

feat: create hooks for different relevant tasks

297285e

feat: switch relevant commits dropdown to use hooks

c37f507

feat: implement metadata for stepback

a6f9d3c

ZackarySantana self-assigned this Mar 12, 2024

ZackarySantana added 6 commits March 15, 2024 11:28

feat: improve hooks slightly

3ae5dcd

fix: renamed to breaking task

fa32bef

fix: make stepback metadata more independent

5d7fbd1

feat: add some dummy tests for useBreakingTask hook

254da0b

feat: enable usebreakingtask test file

119e3c4

feat: extract render logic to prevent hook mismatch

63e2d5b

ZackarySantana changed the title ~~DEVPROD-4608 Add stepback to the metadata of tasks [not ready for review]~~ DEVPROD-4608 Add stepback to the metadata of tasks Mar 18, 2024

ZackarySantana requested a review from a team March 18, 2024 13:56

ZackarySantana added 2 commits March 18, 2024 12:47

Merge branch 'main' into DEVPROD-4608

b59ba83

feat: switch from tsx to ts

2f988c7

minnakt suggested changes Mar 20, 2024

View reviewed changes

ZackarySantana added 2 commits March 20, 2024 10:29

refactor: improve stepback comments

2ffa66a

fix: fix useBreakingTask test to await for expect

a898f0d

ZackarySantana added 6 commits March 20, 2024 12:32

feat: use existing components for stepback component

97307bd

feat: use existing toast context and test for toast error to be dispa…

4e0fafc

…tched

feat: add tests for other new hooks

c63b568

refactor: remove unused component

752a05c

fix: remove unused imports

cec293f

Merge branch 'main' into DEVPROD-4608

a52782f

ZackarySantana requested a review from minnakt March 20, 2024 17:44

minnakt suggested changes Mar 20, 2024

View reviewed changes

ZackarySantana added 2 commits March 21, 2024 09:21

fix: remove async for non-async test functions

4760e38

refactor: improve readability with comments and names

7b50505

ZackarySantana added 2 commits March 21, 2024 09:36

fix: wrap stepback in metadata item

efde83b

feat: pass task from actionbuttons to relevant commits dropdown

b007675

ZackarySantana requested a review from minnakt March 21, 2024 14:13

minnakt suggested changes Mar 21, 2024

View reviewed changes

feat: nits/changes

3bc6f3b

ZackarySantana requested a review from minnakt March 21, 2024 16:39

minnakt approved these changes Mar 21, 2024

View reviewed changes

ZackarySantana added 4 commits March 21, 2024 13:58

fix: oopsy

7bb69c3

fix: wrapping for text

2b5787e

fix: rename task ids to match

bbfc905

fix: update snapshots

e74453a

ZackarySantana merged commit fdf7d25 into evergreen-ci:main Mar 21, 2024
4 checks passed

ZackarySantana deleted the DEVPROD-4608 branch March 21, 2024 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEVPROD-4608 Add stepback to the metadata of tasks #2299

DEVPROD-4608 Add stepback to the metadata of tasks #2299

ZackarySantana commented Mar 12, 2024 •

edited

Loading

cypress bot commented Mar 12, 2024 •

edited

Loading

ZackarySantana commented Mar 19, 2024

minnakt left a comment

minnakt Mar 20, 2024

minnakt Mar 20, 2024

minnakt Mar 20, 2024

minnakt Mar 20, 2024

ZackarySantana Mar 20, 2024

minnakt Mar 20, 2024

minnakt Mar 20, 2024

minnakt Mar 20, 2024

ZackarySantana Mar 20, 2024

ZackarySantana commented Mar 20, 2024

minnakt left a comment

minnakt Mar 20, 2024

ZackarySantana Mar 21, 2024

minnakt Mar 20, 2024

minnakt Mar 20, 2024

minnakt Mar 20, 2024

minnakt Mar 20, 2024

ZackarySantana Mar 21, 2024

minnakt Mar 20, 2024

minnakt Mar 20, 2024

minnakt Mar 21, 2024

minnakt Mar 21, 2024

minnakt Mar 21, 2024

minnakt Mar 21, 2024

ZackarySantana Mar 21, 2024

minnakt left a comment

minnakt Mar 21, 2024

minnakt Mar 21, 2024

ZackarySantana Mar 21, 2024

		@@ -0,0 +1,180 @@
		import { ProviderWrapper } from "components/HistoryTable/hooks/test-utils";

		// nextStepbackTaskId is set when the next task in stepback in known, in the beginning
		// of stepback, it is known right away. In the rest of stepback, it is not.

DEVPROD-4608 Add stepback to the metadata of tasks #2299

DEVPROD-4608 Add stepback to the metadata of tasks #2299

Conversation

ZackarySantana commented Mar 12, 2024 • edited Loading

Description

Screenshots

In progress

Completed

Hovering dropdown

Testing

cypress bot commented Mar 12, 2024 • edited Loading

5 flaky tests on run #16261 ↗︎

Review all test suite changes for PR #2299 ↗︎

ZackarySantana commented Mar 19, 2024

minnakt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZackarySantana commented Mar 20, 2024

minnakt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minnakt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZackarySantana commented Mar 12, 2024 •

edited

Loading

cypress bot commented Mar 12, 2024 •

edited

Loading