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

test: Improve coverage for Saxon Scales. #154

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

marinojoey
Copy link
Contributor

@marinojoey marinojoey commented Sep 1, 2023

I was trying to make a test for '7a/7b' but it was erroring that it was not an expected Saxon Grade. I believe this is a problem with the regex in src/scales/saxon.ts. From some simple testing, this regex matches '7a/7b': 1/^((([7-9]|1[0-3])([a-c]))|([1-6])|([7-9]|1[0-3])(([a-c])\/([7-9]|1[0-3])([a-c])))$/i.

For now I pushed up tests that pass inside one commit. For the sake of performance, they were specifically written to improve coverage and nothing else (i.e. __tests__/saxon.ts is not a comprehensive test suite for the Saxon system).

The other commit includes a potential solution to the grade regex.

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

Coverage report

Statements coverage not met for global: expected <=999999 not covered statements, but got 549

St.
Category Percentage Covered / Total
🟢 Statements
86.87% (+0.47% 🔼)
549/632
🟡 Branches
65.9% (+0.58% 🔼)
114/173
🟢 Functions
80.15% (+0.74% 🔼)
109/136
🟢 Lines
86.08% (+0.51% 🔼)
507/589

Test suite run success

258 tests passing in 17 suites.

Report generated by 🧪jest coverage report action from 1b8f6f3

Copy link
Collaborator

@musoke musoke 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 the PR! We already have some tests in src/__tests__/scales/saxon.ts, but they should be in the directory you used. Could you move the existing tests to the location of your current tests and add any new cases to that?

@vnugent
Copy link
Contributor

vnugent commented Oct 18, 2023

Could you move the existing tests to the location of your current tests and add any new cases to that?

@marinojoey can you finish this PR?

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

Successfully merging this pull request may close these issues.

3 participants