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

Add cypress tests for alphabetical index Vue component #1688

Merged
merged 7 commits into from
Oct 3, 2024

Conversation

UnniKohonen
Copy link
Contributor

Reasons for creating this PR

Alphabetical index has no cypress tests, this PR adds them.

Link to relevant issue(s), if any

Description of the changes in this PR

  • Added cypress tests for tab-alpha Vue component
  • Added a new test vocab for testing loading on scroll

Addresses requirement 6 in #1563

Known problems or uncertainties in this PR

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@UnniKohonen UnniKohonen added this to the 3.0 milestone Oct 2, 2024
@UnniKohonen UnniKohonen self-assigned this Oct 2, 2024
@UnniKohonen UnniKohonen requested a review from osma October 2, 2024 12:46
Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.66%. Comparing base (12af8f3) to head (ade6442).
Report is 9 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1688      +/-   ##
============================================
- Coverage     70.70%   70.66%   -0.05%     
  Complexity     1651     1651              
============================================
  Files            33       33              
  Lines          4332     4332              
============================================
- Hits           3063     3061       -2     
- Misses         1269     1271       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

The existing tests look good. A few thoughts below.

I think there could also be a test that checks for altLabels that should appear in the index. For example in the YSO Archaeology test vocabulary, there is this altLabel entry: artefacts → articles (inanimate objects)

Also, would it be a good idea to check the links to concepts as well? For example verify that the target URL is correct and/or simulate a click and verify that the concept is loaded. OTOH, there are tests like this already in vocab-home.cy.js so maybe there is no need to duplicate those. Maybe the vocab-home.cy.js tests could be moved alongside these, since they exercise the hierarchy sidebar?

All the new sidebar-related tests are now in sidebar.cy.js. This file may grow quite big since the sidebar has several components and more will be added later. Would it make sense to split it already now into e.g. sidebar-alpha.cy.js, sidebar-hierarchy.cs.js etc?

@UnniKohonen UnniKohonen requested a review from osma October 3, 2024 11:44
Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

sonarcloud bot commented Oct 3, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
3 Security Hotspots

See analysis details on SonarCloud

@UnniKohonen UnniKohonen merged commit 75e8c48 into main Oct 3, 2024
9 of 10 checks passed
@UnniKohonen UnniKohonen deleted the alphabetical-index-cypress-tests branch October 3, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done (verified in test.dev.finto.fi, set Milestone 3.0 for both issue & PR)
Development

Successfully merging this pull request may close these issues.

2 participants