Skip to content

Conversation

@oliverklee
Copy link
Collaborator

Part of #757

@coveralls
Copy link

coveralls commented Feb 25, 2025

Coverage Status

coverage: 54.627% (+0.4%) from 54.259%
when pulling a69d2cb on task/tests/get-all-rulesets
into 4384bbe on main.

@oliverklee oliverklee force-pushed the task/tests/get-all-rulesets branch from 52bd86e to d097e0b Compare February 25, 2025 10:25
Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

The tests look fine. I've suggested improvements for some of the test method names, where I found them difficult to understand (and, equally, difficult to rewrite).

@oliverklee oliverklee force-pushed the task/tests/get-all-rulesets branch from d097e0b to 41a4348 Compare February 25, 2025 10:46
@oliverklee oliverklee force-pushed the task/tests/get-all-rulesets branch from 41a4348 to a7747f4 Compare February 25, 2025 10:51
Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

Trying to test that RuleSets are being set does create some awkwardness re test method names. It would much easier if they were called RuleCollections. Or we used German - the two meanings of the English word 'set' don't seem to have any correlation.

I think we've done the best we reasonably can :)

@JakeQZ JakeQZ merged commit 00e23e9 into main Feb 26, 2025
21 checks passed
@JakeQZ JakeQZ deleted the task/tests/get-all-rulesets branch February 26, 2025 00:06
@oliverklee
Copy link
Collaborator Author

Yes, naming is one of the two hard things in computer science … 😉

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.

4 participants