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

tests: news: refactor, add cases #989

Closed
wants to merge 9 commits into from
Closed

Conversation

thesamesam
Copy link
Member

This would've caught the various news regressions we had over the last year.

This is some prep work to modernise the tests a bit
by using dataclasses and idiomatic Python string/template
substitution.

The key point here is the changes facilitate testing
with various combinations of fields, including multiple
e.g. Display-If-Installed, which we couldn't do cleanly
before.

Bug: https://bugs.gentoo.org/889330
Signed-off-by: Sam James <sam@gentoo.org>
Previously, we'd ignore invalid news items in these tests
because they're intended to check for the respective
'relevance' field. Fix that.

Bug: https://bugs.gentoo.org/889330
Signed-off-by: Sam James <sam@gentoo.org>
Bug: https://bugs.gentoo.org/889330
Signed-off-by: Sam James <sam@gentoo.org>
Add a basic test case for when no filter fields are used
(no Display-If-*).

Bug: https://bugs.gentoo.org/889330
This shows that our current Display-If-Installed test
doesn't work properly, as it'll pass with any package
value.

Bug: https://bugs.gentoo.org/889330
Signed-off-by: Sam James <sam@gentoo.org>
It's been TODO for many years (see a4acda0)
and it covered up a problem with Display-If-Installed's test not actually
asserting if the package was installed or not.

Now e.g. dbapi.match() gives a proper result.

Bug: https://bugs.gentoo.org/889330
Signed-off-by: Sam James <sam@gentoo.org>
This eliminates a lot of boilerplate from each test.

Bug: https://bugs.gentoo.org/889330
Signed-off-by: Sam James <sam@gentoo.org>
Copy link
Contributor

@rndxelement rndxelement left a comment

Choose a reason for hiding this comment

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

Greetings, the news item test rework looks great! I came across a few questions and thought that I might share them through this review

lib/portage/tests/news/test_NewsItem.py Show resolved Hide resolved
lib/portage/tests/news/test_NewsItem.py Outdated Show resolved Hide resolved
lib/portage/tests/news/test_NewsItem.py Show resolved Hide resolved
These tests would've caught the 3 previous regressions we had in the news feature
over the last year.

Verified by reverting each of the relevant fix commits (see below) and confirming
the relevant tests start to fail then pass again once the fix is cherry-picked in isolation.

See: 0e56f99
See: f1d98b6
See: 1ffaa70
Bug: https://bugs.gentoo.org/857669
Bug: https://bugs.gentoo.org/889330
Signed-off-by: Sam James <sam@gentoo.org>
Bug: https://bugs.gentoo.org/889330
Signed-off-by: Sam James <sam@gentoo.org>
@thesamesam
Copy link
Member Author

Thanks for the review @rndxelement!

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