Skip to content
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

Flaky assertion fix #7269

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

matejnesuta
Copy link
Contributor

@matejnesuta matejnesuta commented Apr 10, 2024

Describe the change

Related to #7237.

I have not found a reliable way to reproduce the failure, but I have encountered it multiple times both on upstream OCP and Kind. Occasionally, the code inside the Mocha hook was not executed at all and the test passed even though it should detect a failure. This piece of code is tied to most of the CRD validation tests in Cypress.

Steps to test the PR

Create a minimal Istio gateway in Kiali and insert this scenario in the istio_config.feature file:

  @bookinfo-app
  @selected
  Scenario: delete me later
    When user selects the "bookinfo" namespace
    Then the "foo" "Gateway" of the "bookinfo" namespace should have a "danger"

Run the yarn cypress:selected command and the test should fail.

When doing the same procedure against the master branch, there is a chance that the scenario will pass instead of failing.

Issue reference

#7237

@matejnesuta matejnesuta added the test: front-end/cypress PR adds/updates front-end tests (unit and/or cypress automation ) label Apr 10, 2024
@matejnesuta
Copy link
Contributor Author

KIA0505 validation is a flaky test, but the other two scenarios are failing consistently on my local Kind.

@matejnesuta
Copy link
Contributor Author

The cleanup hook was not working, so a fix of that will hopefully resolve some of the flakes. KIA1102 is probably a bug (see #7275).

@@ -522,21 +522,19 @@ Then('the AuthorizationPolicy should have a {string}', function (healthStatus: s
Then(
'the {string} {string} of the {string} namespace should have a {string}',
(crdInstanceName: string, crdName: string, namespace: string, healthStatus: string) => {
it('loading config list', { retries: 3 }, () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to leave the "retries 3" as this is done to make sure that newly created objects appear in cache and are listed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a gherkin step before this assertion in every scenario, which selects a namespace and waits until loading finishes. Shouldn't this be sufficient enough?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that step is not enough because it checks the load not the content update with newly created objects, which loading can take some time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced the original code with my own mechanism using recursion. The cy.wait() interval can be tweaked further.

@matejnesuta matejnesuta requested a review from hhovsepy May 22, 2024 15:06
.then(() => {
if (!found) {
cy.wait(40000);
waitUntilConfigIsVisible(attempt - 1, crdInstanceName, crdName, namespace);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matejnesuta does the tests eventually stop looping if there is error?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test: front-end/cypress PR adds/updates front-end tests (unit and/or cypress automation )
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive assertion in Cypress test related to Istio config validation
3 participants