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

Use skip markers in tests #4188

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

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Jan 19, 2023

From #3790

We can use the more conventional @unittest.skipIf or self.skipTest markers to skip tests instead of C&P of conditions which might be wrong (some were)
An approach taken from IIRC PyTorch is to define own decorators requiresFoo as a wrapper around a specific skipIf which is reusable and easier to understand.

Only issue I can see is with the change to CI to ignore all skipped tests as now that I look at it previously we were able to selectively ignore some skipped tests.
So maybe we should not allow any skipped tests on CI which usually means installing additional Python packages. And for the tests requiring a github token we then need to resort back to the old approach but maybe we can still use a decorator here

@boegel boegel added the tests label Jan 21, 2023
@boegel boegel added this to the next release (4.7.1?) milestone Jan 21, 2023
@@ -212,7 +212,7 @@ jobs:
# run test suite
python -O -m test.framework.suite 2>&1 | tee test_framework_suite.log
# try and make sure output of running tests is clean (no printed messages/warnings)
IGNORE_PATTERNS="no GitHub token available|skipping SvnRepository test|requires Lmod as modules tool|stty: 'standard input': Inappropriate ioctl for device|CryptographyDeprecationWarning: Python 3.[56]|from cryptography.*default_backend|CryptographyDeprecationWarning: Python 2|from cryptography.utils import int_from_bytes|Blowfish|GC3Pie not available, skipping test"
IGNORE_PATTERNS="no GitHub token available|skipping SvnRepository test|requires Lmod as modules tool|stty: 'standard input': Inappropriate ioctl for device|CryptographyDeprecationWarning: Python 3.[56]|from cryptography.*default_backend|CryptographyDeprecationWarning: Python 2|from cryptography.utils import int_from_bytes|Blowfish|GC3Pie not available, skipping test|\.s+"
Copy link
Member

Choose a reason for hiding this comment

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

Adding \s+ to IGNORE_PATTERNS defeats the whole purpose of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pythons unittest will print a literal dot for each test executed and a literal s for each test skipped.

I did add \.s+ not \s+ to the patterns, i.e. I want to ignore a dot followed by one or more "s" as otherwise those skipped tests would make CI fail. See my last paragraph in the PR description about a discussion starter on this.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right, I totally misinterpreting this, sorry... Makes sense

@boegel
Copy link
Member

boegel commented Feb 1, 2023

Only issue I can see is with the change to CI to ignore all skipped tests as now that I look at it previously we were able to selectively ignore some skipped tests. So maybe we should not allow any skipped tests on CI which usually means installing additional Python packages. And for the tests requiring a github token we then need to resort back to the old approach but maybe we can still use a decorator here

@Flamefire How about only allowing \.s+ in the output when running the test suite in a PR?
Because only then should we allow skipped tests (because the GitHub token is missing)...

So we should conditionally extend IGNORE_PATTERNS with |\.s+ if the test suite is being triggered by a push to develop branch (I think)...

@Flamefire
Copy link
Contributor Author

So we should conditionally extend IGNORE_PATTERNS with |\.s+ if the test suite is being triggered by a push to develop branch (I think)...

I guess you meant to insert a NOT somewhere as no tests should be skipped on push to develop.

How about only allowing \.s+ in the output when running the test suite in a PR?
Because only then should we allow skipped tests (because the GitHub token is missing)...

Makes sense, however we should be more selective here. I hence added a dedicated decorator for that which doesn't produce output for PRs (i.e. silently succeeds) and make use of $FORCE_EB_GITHUB_TESTS to ensure github tests are run when the token is expected to be there.

@Flamefire Flamefire force-pushed the skip-markers branch 5 times, most recently from f76b45c to 65320fb Compare February 2, 2023 12:20
@Flamefire Flamefire mentioned this pull request Feb 2, 2023
2 tasks
@Flamefire Flamefire changed the title Use skip markers and skipTest in tests Use skip markers in tests Feb 2, 2023
@Flamefire Flamefire force-pushed the skip-markers branch 7 times, most recently from 05c14fe to 2a38d1e Compare February 7, 2023 15:08
@easybuilders easybuilders deleted a comment from boegelbot Feb 15, 2023
@easybuilders easybuilders deleted a comment from boegelbot Feb 15, 2023
@easybuilders easybuilders deleted a comment from boegelbot Feb 15, 2023
@Flamefire Flamefire force-pushed the skip-markers branch 2 times, most recently from 4040414 to 149f7ec Compare May 11, 2023 08:56
The pip-installable pysvn is incomplete and the full one is not easy to install.
Simply return if `$TEST_EASYBUILD_MODULES_TOOL` is not set to Lmod.
This avoids any "skip" output on CI
For PRs silently skip the tests when no token is available else use a skip.
On GHA force enable the tests when the source repo is 'easybuilds' so a
token is expected to be available.
As that is used a lot use that for better describing the purpose.
GC3Pie on Python 3.11 does not work: gc3pie/gc3pie#674
So avoid the skip mark for that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants