feat(frontend): Support showing Omitted node phase with new Argo version

[zijianjoy]

Description of your changes:

Issue #5322

With PR #5266 we have upgraded to use the new Argo version. However, there is a new NodePhase with OMITTED in a finished pipeline. We shouldn't show a tail for Omitted node, and should provide an Icon to show the Omitted state. See screenshot:

Checklist:

• The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

• Do you want this pull request (PR) cherry-picked into the current release branch?

  Learn more about cherry-picking updates into the release branch.

[Bobgy]

Awesome! Thank you for the quick fix!

[Bobgy]

Besides that, I feel the icon might be ambiguous. The steps are omitted, but not failed, the icon makes it look like they failed.

What do you think about https://material.io/resources/icons/?icon=do_not_disturb_on&style=baseline? I don't have a good idea of which icon might be a better fit. You can find other icons too.

[zijianjoy]

Besides that, I feel the icon might be ambiguous. The steps are omitted, but not failed, the icon makes it look like they failed.

What do you think about https://material.io/resources/icons/?icon=do_not_disturb_on&style=baseline? I don't have a good idea of which icon might be a better fit. You can find other icons too.

That makes sense, thank you for the suggestion! I changed the icon based on research on material io page. Here is what I land on, and the reason is that this icon gives the impression that this step hasn't been executed.

For reference: Argo workflow didn't differentiate this new state from init state on their icon list: https://github.com/argoproj/argo-workflows/blob/9319c074e742c5d9cb97d6c5bbbf076afe886f76/ui/src/app/shared/utils.ts#L6-L26. If they do, I am leaning towards using something similar. Since they don't have an icon, we can pick one we think makes most sense.

[Bobgy]

Thank you!
These all makes sense to me!

The only blocking issue is that the generated snapshot is too large and unreadable. I think you should mock all the icon components. Do you know how to do that with jest? There are some examples in the repo.

EDIT: here's the best example:

pipelines/frontend/src/pages/RunDetails.test.tsx

Line 41 in 26bd912

jest.mock('../components/Graph', () => {

[zijianjoy]

The only blocking issue is that the generated snapshot is too large and unreadable. I think you should mock all the icon components. Do you know how to do that with jest? There are some examples in the repo.

EDIT: here's the best example:

pipelines/frontend/src/pages/RunDetails.test.tsx

Line 41 in 26bd912

jest.mock('../components/Graph', () => {

Thank you so much Yuan! That is a very useful reference. I have done some research on the usage and our test cases, I found that the most verbose part of this element is in Tooltip, which has a lot of props. But the IconComponent itself is very short, so I changed the test to validate the changing part of statusToIcon(): which is the content of IconComponent.

Furthermore, I want to validate the hover text for each status is accurate as well. However, there is a blocking issue on jest which I cannot add test for, until we upgrade jest to v26.4.4, which means upgrading react-scripts to v4+: mui/material-ui#15726.

Since it will fail a lot of tests after upgrading react-scripts, I put this on hold for now. Putting the test I want to add here.

    it('react testing for ERROR phase', async () => {
      const { findByText, getByTestId } = render(
        statusToIcon(NodePhase.ERROR),
      );

      fireEvent.mouseOver(getByTestId('node-status-sign'));
      findByText('Error while running this resource');
    });

I have validated this is working after locally using react-scripts v4+.

[Bobgy]

/lgtm
/approve

[Bobgy]

this is an unstable finder, if any other span element shows up, it'll break.

I'd suggest this blogpost https://kentcdodds.com/blog/making-your-ui-tests-resilient-to-change.
There are some general advice for choosing a selector, it should be

1. reflect how users find elements
2. if not feasible, add a unique data-testid

This is not a blocker for this PR, your improvements have been pretty good!

[zijianjoy]

Thank you for the suggestion! I agree with the testing approach which is based on user visible element. The current implementation is temporary, and we should use the react testing library (commented test) after version upgrade, so we can abandon this test.

[google-oss-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• frontend/OWNERS [Bobgy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment