-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
TST/CI: Unify CI test scripts (single and multi) and simplify the system #23924
Conversation
Hello @datapythonista! Thanks for submitting the PR.
|
fi | ||
|
||
|
||
echo "uploading coverage" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to remove this echo? Figure it doesn't hurt to keep around to give an indication that we've gotten to this point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, got lost when moving the code
Codecov Report
@@ Coverage Diff @@
## master #23924 +/- ##
===========================================
- Coverage 92.3% 42.45% -49.85%
===========================================
Files 161 161
Lines 51551 51549 -2
===========================================
- Hits 47585 21886 -25699
- Misses 3966 29663 +25697
Continue to review full report at Codecov.
|
NUM_JOBS=2 | ||
fi | ||
|
||
pytest -m "$TYPE_PATTERN$PATTERN" -n $NUM_JOBS -s --strict --durations=10 --junitxml=test-data-$TYPE.xml $TEST_ARGS $COVERAGE pandas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have TEST_ARGS here, though you removed it elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to get rid of it, but it's needed in the numpydev build for the -W error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any other way to do that. this makes it pretty clunky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to take a look at pytest concurrency in a different PR.
I think it should be a simpler way to avoid running the simple
tests in parallel, without having to call pytest twice. I think that would be cleaner, and we can simplify all this even more.
Also, I want to see how many processes we have, and not call always for 2 (which I assume it's the number of cores in Travis), but instead use all the available (I think that should be as easy as -n auto
, but I want to take a look at how many cores are detected in azure, and how many makes sense to use).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the single tests must remain though
we got failures because these are using single system resources
pandas/conftest.py
Outdated
@@ -28,28 +28,13 @@ | |||
|
|||
|
|||
def pytest_addoption(parser): | |||
parser.addoption("--skip-slow", action="store_true", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wouldn't remove these, these are useful from the command line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why to use pytest --skip-slow
instead of pytest -m "no slow"
? Not a big deal to restore them, but I personally think that they are more complexity than value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this is a pretty common idiom. I don't see value in removing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@datapythonista can you restore these
pandas/conftest.py
Outdated
@@ -28,28 +28,13 @@ | |||
|
|||
|
|||
def pytest_addoption(parser): | |||
parser.addoption("--skip-slow", action="store_true", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@datapythonista can you restore these
looks like the doc build failed: https://travis-ci.org/pandas-dev/pandas/builds/461388989?utm_source=github_status&utm_medium=notification i restarted, let's see. |
I didn't see the docs failed. I forgot to skip the tests in the doc build, and it was timing out doing both things. It should be fixed now. |
thanks @datapythonista |
…tem (pandas-dev#23924) * WIP: unifying script_single and script_multi * Unified scripts that run the tests (single and multi) * pending fixes * logging when we upload the coverage * Restoring pytest --skip-slow... options * Not running tests on doc build
…tem (pandas-dev#23924) * WIP: unifying script_single and script_multi * Unified scripts that run the tests (single and multi) * pending fixes * logging when we upload the coverage * Restoring pytest --skip-slow... options * Not running tests on doc build
…tem (pandas-dev#23924) * WIP: unifying script_single and script_multi * Unified scripts that run the tests (single and multi) * pending fixes * logging when we upload the coverage * Restoring pytest --skip-slow... options * Not running tests on doc build
git diff upstream/master -u -- "*.py" | flake8 --diff
The goal of this PR is:
ci/script_single.sh
andci/script_multi.sh
SLOW=true
and--only-slow
) and using the pytest native syntax insteadupload_coverage.sh
into the new unified script to avoid the extra complexity of having many scripts