-
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
[#1220] Cypress: test ramp chart in zoom view #1930
Conversation
cb9825f
to
59293c9
Compare
59293c9
to
974e4af
Compare
The frontend tests are failing due to the new merge commits made and can be fixed by merging #1882. |
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! I think that it would be important to add tests to test for the correctness of the filtering of commit messages, as these tests rely on the correctness of those (as you mentioned) e.g. setting date/zoom range in summary panel correctly filters commits in zoom panel.
I'll open a separate issue/PR for that!
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.
@ckcherry23 More test cases are always welcomed and the test cases added are certainly useful. However, I think other test cases could be added to make this test file more powerful. I am not too sure if it is possible to cover cases regarding the width, start/end date, link to the commit, relative distance between each ramp. Would you mind exploring if any of those are possible?
If there are other areas of testing that you think we can add, feel free to add on. If the ones that I mentioned above are not possible to do/not worth the effort, do share below and feel free to discuss!
…test-zoomview-rampChart
…23/RepoSense into test-zoomview-rampChart
Though the initial purpose of this PR was just to test if the ramp chart matches the selected timeframe, I agree that more tests need to be added for the ramp charts. However, many of these properties are difficult to test and some values like ramp width and distance may even change between platforms, even if the viewport size is the same. While I have added a few test cases with hardcoded expected values, I will explore if there are ways to test these in a more robust manner. |
…23/RepoSense into test-zoomview-rampChart
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! I do agree with your point. But I think these hardcoded test cases do add value as a sanity check for future updates.
The following links are for previewing this pull request:
|
Fixes #1220
Proposed commit message
Other information
For this PR, I check whether the ramp chart corresponds with the selected timeframe by checking if the number of ramps is equal to the number of commit messages. Therefore the confidence level of these tests depends on the correctness of the filtering of commit messages.
For the other tests, one downside is that a lot of them are hardcoded or skipped as the values cannot be correctly estimated using conventional Cypress test methods. I will be exploring the use of jQuery methods to make these test cases more robust.