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

[#2195] Add cypress test for show tag when grouping by author and by none #2197

Merged
merged 10 commits into from
May 12, 2024

Conversation

jonasongg
Copy link
Contributor

@jonasongg jonasongg commented Apr 30, 2024

Fixes #2195

Proposed commit message

Add cypress test for show tag when grouping by author and by none

Currently, there are no Cypress tests to test the behaviour of the show
tag feature when the report is grouped by author or grouped by none.

Let's add a test for this to ensure the feature is working properly.

Other information

@jonasongg jonasongg requested review from a team April 30, 2024 10:07
Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the author-config.csv to test this. I've left a few comments about the tests.

Also, I just realised that the tags feature does not work as expected when using the Group by None option. Each author gets all of the tags of the repo instead of only the ones they tagged. Let's create a new issue for this.

Screenshot 2024-05-01 at 2 03 52 PM

frontend/cypress/tests/chartView/chartView_showTags.cy.js Outdated Show resolved Hide resolved
frontend/cypress/config/author-config.csv Outdated Show resolved Hide resolved
@jonasongg
Copy link
Contributor Author

jonasongg commented May 1, 2024

@ckcherry23 the bug with group by none actually happened because of a change I made in this PR to fix group by author. i fixed the bug in this PR already, so all group by options should be working correctly. (also added some tests for this!) do i need to create a new issue + PR to address the original bug?

@jonasongg jonasongg requested a review from ckcherry23 May 8, 2024 07:06
@ckcherry23
Copy link
Member

@ckcherry23 the bug with group by none actually happened because of a change I made in this PR to fix group by author. i fixed the bug in this PR already, so all group by options should be working correctly. (also added some tests for this!) do i need to create a new issue + PR to address the original bug?

If everything works as expected now, we do not need a new issue + PR. The current PR seems to have fixed all the issues, is there anything missing?

Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

LGTM @jonasongg! Thanks for making the test cases more robust by comparing chart tags with those on the Zoom tab!

Please update the PR title and description to reflect that both test cases were added ('group by author' and 'group by none').

@ckcherry23 ckcherry23 requested a review from a team May 11, 2024 07:49
@jonasongg jonasongg changed the title Add cypress test for show tag when grouping by author [#2195] Add cypress test for show tag when grouping by author and none [#2195] May 11, 2024
@jonasongg jonasongg changed the title Add cypress test for show tag when grouping by author and none [#2195] Add cypress test for show tag when grouping by author and by none [#2195] May 11, 2024
@jonasongg
Copy link
Contributor Author

@ckcherry23 done, thanks!

@ckcherry23 ckcherry23 changed the title Add cypress test for show tag when grouping by author and by none [#2195] [#2195] Add cypress test for show tag when grouping by author and by none May 11, 2024
Copy link
Contributor

@vvidday vvidday left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@ckcherry23 ckcherry23 merged commit 4735dcf into master May 12, 2024
15 checks passed
Copy link
Contributor

The following links are for previewing this pull request:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Cypress tests for show tags on ramp chart
3 participants