test: improve reliability of /bugs/4641

[tim-lai]

Ref #6001

Description

Refactor existing tests that should avoid timeouts.

In Cypress, the .should command asserts an explicit argument against a single value. In this case, we want to be able to place multiple assertions. Therefore, this change utilizes the .then callback for .its command, to which we can inspect various elements and make multiple assertions. Using a callback also ensures that the request was completed before we make a set of assertions

Note, for 2 of the 3 tests, since the assertion is on a specific known argument and expected value, it is possible to utilize .should, albeit with structural change. e.g.

.its('some.very.specific.target)
.should('eq', value1)
.should('not.have.value', value2)

Motivation and Context

The existing test in /bugs/4641.js intermittently fails on CI.

Fixes #6001

How Has This Been Tested?

updated test passes

Screenshots (if appropriate):

Checklist

My PR contains...

• No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
• Dependency changes (any modification to dependencies in package.json)
• Bug fixes (non-breaking change which fixes an issue)
• Improvements (misc. changes to existing features)
• Features (non-breaking change which adds functionality)

My changes...

• are breaking changes to a public API (config options, System API, major UI change, etc).
• are breaking changes to a private API (Redux, component props, utility functions, etc.).
• are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
• are not breaking changes.

Documentation

• My changes do not require a change to the project documentation.
• My changes require a change to the project documentation.
• If yes to above: I have updated the documentation accordingly.

Automated tests

• My changes can not or do not need to be tested.
• My changes can and should be tested by unit and/or integration tests.
• If yes to above: I have added tests to cover my changes.
• If yes to above: I have taken care to cover edge cases in my tests.
• All new and existing tests passed.

[char0n]

Hi @tim-lai,

In Cypress, the .should command asserts an explicit argument against a single value. In this case, we want to be able to place multiple assertions. Therefore, this change utilizes the .then callback for .its command, to which we can inspect various elements and make multiple assertions. Using a callback also ensures that the request was completed before we make a set of assertions

I'm not sure I fully understand this description. There are couple of claims in it, let me address them and cross reference with Cypress documentation.

.should command asserts an explicit argument against a single value

Yes what you explain here is technically true, but more an implementation detail. From API perspective, it creates an assertions and retries it until the assertion passes or times out.

In this case, we want to be able to place multiple assertions. Therefore, this change utilizes the .then callback for .its command, to which we can inspect various elements and make multiple assertions

This is exactly what cypress documentation tell us not to do:
Note: Prefer .should() with callback over .then() for assertions as they are automatically rerun until no assertions throw within it

Using a callback also ensures that the request was completed before we make a set of assertions

Both .then and .should (with callback) works on yielded value. Cypress is promise aware so the behavior should be equivalent within the context of completion.

Overall I not sure I understand what the change in PR achieves. The only semantic difference that I can see is that we now use .then instead of .should (with callback) and the assertions are not retried inside the .then as it has been when used in .should. If the retries have been the source of the test flakiness then I can understand this PRs changes, if not, then I'm not sure how this change fixes the flakiness.

[tim-lai]

@char0n Thanks for the thoughts. I can try clarifying the PR notes.

Short version, I do believe that the retries from should is the issue, as it eventually cannot retrieve the element itself. Cypress' output suggestion to expect null is incorrect, because we actually want and expect specific fields in the request.

Per the "Notes - Differences" section of Cypress docs (same link), I'm relying on the comment to use .then() to "... do some actions". This is indeed a case of "prefer ... be aware of differences". Specifically, this change waits for .its() to complete before performing a set of assertions within the .then() block. The alternative is to chain each assertion with their own .should(), which I alluded to in the PR notes.

I will say both options is theory, as I haven't been able to locally reproduce the intermittent fail in this test.

Is this a more helpful description?