-
-
Notifications
You must be signed in to change notification settings - Fork 26.2k
CI add codecov to GitHub Action workflow #31941
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
base: main
Are you sure you want to change the base?
Changes from all commits
310a4dd
b915d6d
5bd4b76
83c26bb
5a93069
64ce809
a9f2a7a
1634b47
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,11 +8,11 @@ source build_tools/shared.sh | |
activate_environment | ||
|
||
# Combine all coverage files generated by subprocesses workers such | ||
# such as pytest-xdist and joblib/loky: | ||
# as pytest-xdist and joblib/loky: | ||
pushd $TEST_DIR | ||
coverage combine --append | ||
coverage xml | ||
popd | ||
|
||
# Copy the combined coverage file to the root of the repository: | ||
cp $TEST_DIR/coverage.xml $BUILD_REPOSITORY_LOCALPATH | ||
cp $TEST_DIR/coverage.xml . | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment for potential other reviewers: Maybe GitHub has an equivalent environment variable, but it feels slightly simpler to rely on the assumption that the current working directory is the same as the scikit-learn repo root folder. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,7 @@ if [[ "$COMMIT_MESSAGE" =~ \[float32\] ]]; then | |
export SKLEARN_RUN_FLOAT32_TESTS=1 | ||
fi | ||
|
||
CHECKOUT_FOLDER=$PWD | ||
mkdir -p $TEST_DIR | ||
cp pyproject.toml $TEST_DIR | ||
cd $TEST_DIR | ||
|
@@ -42,19 +43,19 @@ show_installed_libraries | |
TEST_CMD="python -m pytest --showlocals --durations=20 --junitxml=$JUNITXML -o junit_family=legacy" | ||
|
||
if [[ "$COVERAGE" == "true" ]]; then | ||
# Note: --cov-report= is used to disable to long text output report in the | ||
# Note: --cov-report= is used to disable too long text output report in the | ||
# CI logs. The coverage data is consolidated by codecov to get an online | ||
# web report across all the platforms so there is no need for this text | ||
# report that otherwise hides the test failures and forces long scrolls in | ||
# the CI logs. | ||
export COVERAGE_PROCESS_START="$BUILD_SOURCESDIRECTORY/.coveragerc" | ||
export COVERAGE_PROCESS_START="$CHECKOUT_FOLDER/.coveragerc" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar comment as https://github.com/scikit-learn/scikit-learn/pull/31941/files#r2276870682. It feels slightly preferable to rely on the assumption that the current working directory is the scikit-learn repo root folder. |
||
|
||
# Use sys.monitoring to make coverage faster for Python >= 3.12 | ||
HAS_SYSMON=$(python -c 'import sys; print(sys.version_info >= (3, 12))') | ||
if [[ "$HAS_SYSMON" == "True" ]]; then | ||
export COVERAGE_CORE=sysmon | ||
fi | ||
TEST_CMD="$TEST_CMD --cov-config='$COVERAGE_PROCESS_START' --cov sklearn --cov-report=" | ||
TEST_CMD="$TEST_CMD --cov-config='$COVERAGE_PROCESS_START' --cov=sklearn --cov-report=" | ||
Comment on lines
-57
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This diff was necessary to fix the order of the cli arguments. For some reason, this was evaluated as |
||
fi | ||
|
||
if [[ "$PYTEST_XDIST_VERSION" != "none" ]]; then | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,5 +18,7 @@ dependencies: | |
- pip | ||
- ninja | ||
- meson-python | ||
- pytest-cov | ||
- coverage | ||
- pip | ||
- ccache |
Uh oh!
There was an error while loading. Please reload this page.
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 guess one thing to think about for potential reviewers is whether we are OK with using an external GitHub action. Full disclosure: a few years ago the Codecov uploader was compromised see this.
The only secret being exposed would be the Codecov token. I am not sure how much harm can be done with this token (upload fake coverage reports to Codecov sure but something else?).
An alternative would be to use our Azure script
build_tools/azure/upload_coverage.sh
build_tools/azure/upload_codecov.sh
but it would need to be adapted for GitHub, it uses Azure-specificBUILD_
environment variables.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.
It is the
build_tools/azure/upload_codecov.sh
file.