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

Some unit test improvements #6220

Merged
merged 12 commits into from
Feb 23, 2024
Merged

Conversation

distantnative
Copy link
Member

@distantnative distantnative commented Jan 28, 2024

Fixes

  • Fixed fallback null value for Text\KirbyTag::parent()

Housekeeping

  • Added more explicit coverage annotations to Options, Query, Text and partially Toolkit packages tests, including additional tests for uncovered code

@distantnative distantnative added the type: tests 🧪 Is about missing tests; increases test coverage or improves tests label Jan 28, 2024
@distantnative distantnative added this to the 4.2.0 milestone Jan 28, 2024
@lukasbestle
Copy link
Member

lukasbestle commented Jan 29, 2024

About the files in the config dir: Unfortunately no, such a feature is missing in PHPUnit, which is the reason why we don't use @covers annotations for those methods at the moment: sebastianbergmann/phpunit#3794

About coverage in general going down: I'd say if the new annotation is correct (in that it declares the method(s) that are actually being tested with assertions), then it's fine if coverage goes down as it helps us to see areas of the code that are not properly tested. Even better would of course be to add tests for those now uncovered methods. I think we need to keep in mind that the coverage percentage is also relevant for marketing (e.g. we use it on the "for developers" page). What I wouldn't do is to add @coversNothing to the config tests before PHPUnit supports declaring coverage for those. Otherwise we will no longer know which parts of the config dir we test at all.

@distantnative
Copy link
Member Author

The problem is that often those test for config or other non-class code also touch a dozen other classes etc - so without coversNothing we also have spillover into many other classes.

@lukasbestle
Copy link
Member

That is true. But I feel we need a solution in PHPUnit then to be able to do it properly. Before we have that, I'd rather exclude the whole config dir from the coverage allowlist. But that's also not great as a lot of our logic happens there that we need to test.

So TBH I feel like keeping the status quo is overall the best option.

@lukasbestle
Copy link
Member

A hack could be to put all these config tests into a separate @group so we can test them separately without destroying the coverage stats of the strict tests.

@distantnative distantnative added the needs: changes 🔁 Implement any requested changes to proceed label Jan 30, 2024
@distantnative distantnative removed the needs: changes 🔁 Implement any requested changes to proceed label Feb 2, 2024
@distantnative distantnative requested a review from a team February 2, 2024 23:05
Copy link
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

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

Have not reviewed all changes in detail, but skimmed over the general setup changes. Good idea to split up the KirbyTags tests into separate files. Also great that many of the previously uncovered methods now have tests. 👍

Could you please add/update the first post with the final contents of this PR?

@distantnative distantnative added needs: docs 📝 Requires documentation and removed needs: docs 📝 Requires documentation labels Feb 23, 2024
@distantnative distantnative merged commit 00ce494 into develop-minor Feb 23, 2024
14 checks passed
@distantnative distantnative deleted the tests/unit-tests-improvments branch February 23, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: tests 🧪 Is about missing tests; increases test coverage or improves tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants