-
Notifications
You must be signed in to change notification settings - Fork 154
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
[#1931] Update frontend integration tests for summary and zoom view #2015
[#1931] Update frontend integration tests for summary and zoom view #2015
Conversation
The following links are for previewing this pull request:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this up @germainelee02 and apologies for the late response.
I understand that the nature of these tests might make it necessary to make certain assumptions about the data in the report which might result in some hardcoding of the test actions/values.
Would it be possible to indicate whenever there's an assumption regarding the data in the comments like this? Since the tests that rely on assumptions/hardcoded data might fail in the future if the test repo gets more commits, and it would be easier for future maintainers to know exactly what assumptions are being made in the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on abstracting out common functionalities and the test coverage is also good.
Would it be possible to add test cases for some more functionalities?
- setting the 'granularity' changes the zoom view
- setting the 'granularity' again results in a different zoom view
- filtering by a certain file type changes the zoom view
- filtering by a different file type results in a different zoom view
Should I make a separate PR for that then? @ckcherry23 |
@nseah21 I think this can be done in the same PR, as they have the same purpose of testing whether changes in the summary panel are reflected in the zoom panel. |
Hi, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@germainelee02 I will approve this for now, and add a new issue for the suggested test cases.
Please take up that issue if you would like.
The following links are for previewing this pull request:
|
Fixes #1931