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.

Sign up for GitHub

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

AD test failure fix #227

Merged
merged 4 commits into from
May 17, 2022
Merged

AD test failure fix #227

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aae6056
added wait time before next page in ad creation
amitgalitz May 16, 2022
ea8242f
added some wait before clicking next
amitgalitz May 16, 2022
3b109da
move wait time to timestamp filter function
amitgalitz May 16, 2022
7d9cc08
added wait outside get
amitgalitz May 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cypress/integration/plugins/anomaly-detection-dashboards-plugin/create_detector_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,11 @@ context('Create detector workflow', () => {
);
cy.getElementByTestId('indicesFilter').type(`${TEST_INDEX_NAME}{enter}`);
selectTopItemFromFilter('timestampFilter', false);
cy.wait(500);
Copy link
Member

Choose a reason for hiding this comment

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

😭

Is there an API call that you can intercept and wait until that finishes to continue?

If not, we can also extend the functionality of

opensearch-dashboards-functional-test/cypress/utils/commands.js

Line 70 in 25afb70

Cypress.Commands.add('getElementByTestId', (testId) => {
to allow for { timeout: xxxx } for cy.get [doc]. That way it will navigate to the page and check for longer that the default amount. But if it finds it earlier then it will continue.

Copy link
Member Author

Choose a reason for hiding this comment

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

The second option makes sense to me, I don't think there is an API call that we can intercept easily. However I am still trying to figure out why this really sometimes helps but its pretty hard to replicate locally.

Copy link
Member

Choose a reason for hiding this comment

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

However I am still trying to figure out why this really sometimes helps but its pretty hard to replicate locally.

With the second option? Or just adding a timeout in general?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just adding the timeout itself in this case on the AD test even. For adding the timeout on the get command for all tests, I can easily do that, just seems like it could add a lot of unnecessary time to all tests.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, just to clarify I mean extending it in the way that makes the timeout configurable. RIght now it has the default timeout and when you pass that command if you increase for example to 1 minute ({ timeout: 60000}). It will tell cypress to keep retrying for 1 minute to find the element. But if it finds it in 3 seconds it will continue the test path and not wait for the whole minute. The point where it will increase the time for all tests if all tests actually end up failing to find the element but generally all tests shouldn't experience an increase wait time. This is compared to the arbitrary wait for 3 seconds where it will wait for 3 seconds then move to the next line. But if it actually required 4 seconds then the test will still fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the wait time inside this function as it seemed to help with the issue the most, since it isn't part of getElementByTestId do you recommend moving the wait to the next line of: https://github.com/opensearch-project/opensearch-dashboards-functional-test/pull/227/files#diff-1544ad6eba828cbcff2b11e02c24a29dfa194e5da1cc11de42463f46c3511073R53. For adding the functionality since default is 60s, it would have to be passed with 61000? It doesn't seem to be dependent on the 60 second wait time itself since just 1 second more or even .5 seconds solves the issue which makes me wary of just increasing that wait time. It more seems to be the transition between this line (cy.get('.euiFilterSelectItem').first().click();) and then clicking the next.

Copy link
Member

@ohltyler ohltyler May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Agree with @amitgalitz here, the issue is related to not enough time within selectTopItemFromFilter to load the data and perform the action, such that it gets hung and the runner is not able to click to the next page regardless of the 60s timeout, because the page gets stuck in a weird state. I think the fix makes sense here

Copy link
Member

Choose a reason for hiding this comment

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

Per Cypress guidance, it is anti-pattern to have an explicit wait. Totally understand that this has already been used in other test cases. Just try to see if this is a low hanging fruit to fix. By checking more into the Cypress best practice, I think we can add an assertion for the get which can function as waiting and then try to chain into it.

@amitgalitz @ohltyler

Copy link
Member Author

Choose a reason for hiding this comment

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

Thats a fair point, I'll send an additional PR this week removing the explicit wait and hopefully other uses of it in AD tests


cy.getElementByTestId('defineDetectorNextButton').click();
// wait for a few seconds to move to the next page
cy.wait(3000);
cy.getElementByTestId('defineOrEditDetectorTitle').should('not.exist');
cy.getElementByTestId('configureOrEditModelConfigurationTitle').should(
'exist'
Expand Down