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.

Sign up for GitHub

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

Jump to bottom

Selftest refactoring to fix masked tests #5822

Merged
merged 4 commits into from
Dec 13, 2023
Merged

Selftest refactoring to fix masked tests #5822

merged 4 commits into from
Dec 13, 2023

Conversation

richtja
Copy link
Contributor

@richtja richtja commented Dec 4, 2023

Some functional tests have been masked out due to a safeloader behavior
from #5817. This commit fixes this problem by changing the tests classes
from unittests to avocado instrumented, where it is needed. Also, it
adds warning to avocado documentation about this behavior.

Reference: #5817
Signed-off-by: Jan Richter [email protected]

@richtja richtja requested a review from clebergnu December 4, 2023 16:49
@richtja richtja self-assigned this Dec 4, 2023
@richtja richtja linked an issue Dec 4, 2023 that may be closed by this pull request
Safeloader ignores unittest.TestCase classes when avocado.Test used in same file #5817
Closed
@mr-avocado
Copy link

mr-avocado bot commented Dec 4, 2023

Dear contributor,
Avocado is currently under sprint #103, which is due to release an LTS (Long Term Stability) release.
Please avoid merging changes that do not fall into these categories:

  • Bug fixes
  • Usability Improvements
  • Documentation updates

As for the Avocado utility modules (“avocado.utils”) it is OK to introduce new functionality,
but changes to the existing APIs (including interface and behavior) should be avoided.
These kind of changes should wait until sprint #104.

clebergnu
clebergnu requested changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

@richtja thanks for addressing this issue! I've let a couple of comments which are blockers IMO.

selftests/utils.py Outdated Show resolved Hide resolved
selftests/check.py Outdated Show resolved Hide resolved
@clebergnu clebergnu mentioned this pull request Dec 6, 2023
Closed
@richtja richtja force-pushed the unittest_ignored_fix branch 2 times, most recently from bf906a8 to d75cf74 Compare December 8, 2023 16:55
@richtja richtja requested a review from clebergnu December 8, 2023 17:12
clebergnu
clebergnu requested changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

Hi @richtja,

This looks almost ready to be merged, but there a couple of things to address:

  1. There's a missing white space in the check.py, like in suite static-checks doesn't have 6 testsit has 7.
  2. The order of the commits should be changed in order to help with bisecting. The following order wouldn't break bisecting:
basic selftests after changes fix
add logging stream name to test logs
Selftest refactoring to fix masked tests
selftest size check

One extra nitpick is that the description of the "first" commit there would need a tweak ("basic selftests after changes fix" wouldn't make sense anymore).

@richtja
Copy link
Contributor Author

richtja commented Dec 13, 2023

Hi @richtja,

This looks almost ready to be merged, but there a couple of things to address:

1. There's a missing white space in the `check.py`, like in `suite static-checks doesn't have 6 testsit has 7.`

2. The order of the commits should be changed in order to help with bisecting.  The following order wouldn't break bisecting:

basic selftests after changes fix
add logging stream name to test logs
Selftest refactoring to fix masked tests
selftest size check

One extra nitpick is that the description of the "first" commit there would need a tweak ("basic selftests after changes fix" wouldn't make sense anymore).

Thanks for the review @clebergnu, I will reorder the commits.

@richtja
basic selftests logging updates
793d696 
A few basic tests related to logging has been masked because of avocado-framework#5817.
This is a fix for those tests to catch up with latest changes.

Signed-off-by: Jan Richter <[email protected]>
@richtja
add logging stream name to test logs
bc1f4ce 
In avocado-framework#5665 we introduced the logging stream name in test logs, this added
more possibilities for user how to post-process test logs.
Unfortunately, this change also hided information about modules where
the log has been created. This issue has been solved in avocado-framework#5732.
This commit connects those two solutions by storing stream and module
names.

Referencei: avocado-framework#5665 avocado-framework#5732
Signed-off-by: Jan Richter <[email protected]>
@richtja
Selftest refactoring to fix masked tests
ed96ed5 
Some functional tests have been masked out due to a safeloader behavior
from avocado-framework#5817. This commit fixes this problem by changing the tests classes
from unittests to avocado instrumented, where it is needed. Also, it
adds warning to avocado documentation about this behavior.

Reference: avocado-framework#5817
Signed-off-by: Jan Richter <[email protected]>
Signed-off-by: Cleber Rosa <[email protected]>
@richtja
selftest size check
e02cf0e 
This adds selftest size check to the avocado check.py. This checks the
size of avocado selftests to avoid test issues from avocado-framework#5817.

Signed-off-by: Jan Richter <[email protected]>
@richtja richtja force-pushed the unittest_ignored_fix branch from d75cf74 to e02cf0e Compare December 13, 2023 16:37
clebergnu
clebergnu approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@clebergnu clebergnu merged commit 884a8c4 into avocado-framework:master Dec 13, 2023
61 of 62 checks passed
@richtja richtja deleted the unittest_ignored_fix branch December 14, 2023 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clebergnu clebergnu clebergnu approved these changes

Assignees

@richtja richtja

Labels
refactoring
Projects
Default project
Archived in project
Milestone
#103 (LTS release - Sound of Freedom)
Development

Successfully merging this pull request may close these issues.

Safeloader ignores unittest.TestCase classes when avocado.Test used in same file
2 participants
@richtja @clebergnu