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

Irregularity in behaviour of chartView_zoomFeature Cypress tests #2066

Closed
pratham31012002 opened this issue Nov 8, 2023 · 4 comments · Fixed by #2114
Closed

Irregularity in behaviour of chartView_zoomFeature Cypress tests #2066

pratham31012002 opened this issue Nov 8, 2023 · 4 comments · Fixed by #2114
Labels

Comments

@pratham31012002
Copy link
Contributor

Please include the steps to reproduce the bug.

There seems to be a recurring issue with some tests in chartView_zoomFeature.cy.js, particularly in the changing the righthand boundary, changing the lefthand boundary and changing the righthand and lefthand boundary tests, where these test cases pass/fail non-deterministically after some seemingly unrelated changes are made to the frontend code.

For example, after #2056 was merged, the above mentioned test cases failed in GitHub Actions in the master branch, even though they continued to pass on my local machine (Note that prior to merging it, these test cases were passing in the PR branch).

Screenshot of test cases passing on my local machine: #2057 (comment)

Screenshot of test cases failing on GitHub Actions: #2057 (comment)

The same issue occurred after #2034 was merged, as reported in #2045.

This issue has been created to document this irregularity for future reference, as we might need to find a proper solution for this, if we encounter the same issue again.

What was expected to happen?

These test cases should pass deterministically irrespective of the environment.

Possible cause of this issue

One possible cause could be some underlying resolution dependent issue, which we might want to look into, if required in the future.

Current solution used in #2057 to make the test cases pass

I tried to play around with the boundary points of the zoom feature in the chart view to understand why the test cases were failing. Based on my understanding I came to the following conclusion:

For test changing the righthand boundary, the right boundary point chosen was (185, 20). This point seems to be in between commits [2020-05-23] [#1241] Restore checked file types (#1256): +14 -1 lines(a) and [2020-09-27] Add optional check for quotes in diff file regex (#1330): +1 -1 lines(b).

However, it seems that (185, 20) is very close to the commit (b), and hence, the behaviour of the test case is slightly undeterministic (choosing this range sometimes includes (b) and sometimes doesn't include (b)), and hence it passes on my local machine but CI fails.

Between commit (a) and commit (b), there is a substantial gap (approximately 4 months), hence, we can choose a much conservative boundary point to ensure that the test case passes deterministically. Hence, I changed the boundary point to (170, 20).

Similar arguments for the other 2 test cases.

The same approach was implemented in #2047 to make the test cases pass.

@jq1836
Copy link
Contributor

jq1836 commented Nov 8, 2023

A possible solution to this issue would be to choose a smaller date range when testing the zoom feature which would make the tests less sensitive to resolution related issues. However, this may decrease the sensitivity of the tests in detecting any undesirable deviations from expected behavior. Perhaps somewhere in between, say a range of 10 days may make for a test which would allow us to detect most bugs related to the zoom slice selection while making any resolution related failures of the tests highly unlikely.

@SkyBlaise99
Copy link
Contributor

SkyBlaise99 commented Nov 8, 2023

Hi @pratham31012002 can I just check if changing to 170 is the only thing you did to pass the test cases?

I'm also experiencing the same issue in #2060, and is still failing the test cases even using 170.

Link to changes

Link to CI

image

@pratham31012002
Copy link
Contributor Author

pratham31012002 commented Nov 8, 2023

Hi @SkyBlaise99 , have you pulled from the latest master, the actual values were changed recently, and seem to match the values you are getting, as seen in lines 292, 322, 353 here.

Yeah, changing to 170 was the only thing that I did.

@SkyBlaise99
Copy link
Contributor

@pratham31012002 thanks for the information, syncing with master to see if it fix the issue. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Closed/Completed
Development

Successfully merging a pull request may close this issue.

3 participants