-
Notifications
You must be signed in to change notification settings - Fork 225
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
Run Continuous Integration tests on Github Actions #475
Conversation
Signed-off-by: Wei Ji <[email protected]>
Signed-off-by: Wei Ji <[email protected]>
Signed-off-by: Wei Ji <[email protected]>
Signed-off-by: Wei Ji <[email protected]>
Signed-off-by: Wei Ji <[email protected]>
Try just caching ~/.gmt/cache and ~/.gmt/server.
Co-authored-by: Dongdong Tian <[email protected]>
Good work on the |
FYI, we can't use |
Sounds good. We can always add it back in later if we decide a cache for conda packages is needed. |
OK. I'll revert it after the CI jobs pass. |
run: | | ||
if [ "$RUNNER_OS" == "Windows" ]; then choco install wget; fi # install wget on Windows | ||
mkdir ~/.gmt ~/.gmt/cache ~/.gmt/server | ||
wget --no-check-certificate https://oceania.generic-mapping-tools.org/gmt_hash_server.txt -P ~/.gmt/server/ | ||
for data in earth_relief_01d.grd earth_relief_30m.grd earth_relief_10m.grd; do | ||
wget --no-check-certificate https://oceania.generic-mapping-tools.org/${data} -P ~/.gmt/server/ | ||
done |
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.
We need to change these lines when bumping to 6.1. In 6.1, the earth relief data are stored in ~/.gmt/server/earth/earth_relief/
directory.
# Download remote files, if not already cached | ||
- name: Download remote data (macOS) | ||
shell: bash -l {0} | ||
run: gmt which -Gu @earth_relief_01d @earth_relief_30m @earth_relief_10m @ridge.txt @Table_5_11.txt @tut_bathy.nc @tut_quakes.ngdc @tut_ship.xyz @usgs_quakes_22.txt |
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.
Need to change gmt which -Ga
in GMT 6.1.
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'm OK with the current changes in this PR. Please see if you want to make more changes.
@@ -1,7 +1,7 @@ | |||
# Build, package, test, and clean | |||
PROJECT=pygmt | |||
TESTDIR=tmp-test-dir-with-unique-name | |||
PYTEST_ARGS=--cov-config=../.coveragerc --cov-report=term-missing --cov=$(PROJECT) --doctest-modules -v --mpl --mpl-results-path=results --pyargs | |||
PYTEST_ARGS=--cov-config=../.coveragerc --cov-report=term-missing --cov=$(PROJECT) --doctest-modules -v --mpl --mpl-results-path=results --pyargs ${PYTEST_EXTRA} |
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.
This won't work, because we didn't declare the $PYTEST_EXTRA
environment variable. Also need to remove the $PYTEST_EXTRA
string from the ci_tests.yaml file.
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 think it works. It doesn't have to be an environmental variable. make test
and make test PYTEST_EXTRA="-r P"
are using different pytext arguments.
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.
Now although the tests all pass, we actually can see more "errors" by using -r P
:
______________________ test_call_module_invalid_arguments ______________________
----------------------------- Captured stderr call -----------------------------
gmtinfo [ERROR]: Error for input file: No such file (bogus-data.bla)
________________________ test_call_module_error_message ________________________
----------------------------- Captured stderr call -----------------------------
gmtinfo [ERROR]: Error for input file: No such file (bogus-data.bla)
__________________________ test_extract_region_fails ___________________________
----------------------------- Captured stderr call -----------------------------
pygmt-session [ERROR]: No hidden PS file found
____________________________ test_get_default_fails ____________________________
----------------------------- Captured stderr call -----------------------------
pygmt-session [ERROR]: Syntax error: Unrecognized keyword NOT_A_VALID_NAME
________________________ test_text_nonexistent_filename ________________________
----------------------------- Captured stderr call -----------------------------
text [ERROR]: Error for input file: No such file (notexist.txt)
_______________________________ test_which_fails _______________________________
----------------------------- Captured stderr call -----------------------------
gmtwhich [ERROR]: File 5a9f5f5cb31c4bb99525d0d82eaf0fc6 not found!
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.
Isn't it because the CI (Travis and GH Actions) still uses make test PYTEST_EXTRA="-r P"
? If we run make test
locally after checking out this branch, it probably won't work.
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. Look at the captured stderr above:
gmtinfo [ERROR]: Error for input file: No such file (bogus-data.bla)
This is a GMT error message, but I believe it's already corrected captured in the pygmt test.
I believe the initial idea of PYTEST_EXTRA is that, when we or other users run make test
locally, we won't see these "error" messages, as they're a little confusing (tests pass but there are "errors"). But in CI, make test PYTEXT_EXTRA="-r P"
gives more verbose messages, useful for debugging.
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.
Then we don't actually need to change the Makefile. You'll see that PROJECT
is declared before using $(PROJECT)
, but we don't declare PYTEST_EXTRA
before using $(PYTEST_EXTRA)
, it is effectively blank.
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.
If you run make test
, then PYTEST_EXTRA
is blank; if you run make test PYTEST_EXTRA="-r P"
, then variable PYTEST_EXTRA is defined in shell, and passed into Makefile.
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.
make test
runs:
cd tmp-test-dir-with-unique-name; pytest --cov-config=../.coveragerc --cov-report=term-missing --cov=pygmt --doctest-modules -v --mpl --mpl-results-path=results --pyargs pygmt
and make test PYTEST_EXTRA="-r P"
runs:
cd tmp-test-dir-with-unique-name; pytest --cov-config=../.coveragerc --cov-report=term-missing --cov=pygmt --doctest-modules -v --mpl --mpl-results-path=results --pyargs -r P pygmt
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.
Right, I think I get it now. It wasn't working properly before, but now it does.
Thanks for all the help @seisman. The macOS tests seem to be a bit slower on Github Actions ~10min compared to before on Azure Pipelines ~7min. WIndows seems slightly faster sometimes. I'm sure we'll improve this once we get GMT 6.1 working properly, will merge this now 🎉 |
Description of proposed changes
Consolidate all our Linux/macOS/Windows unit tests into one
.github/workflows/ci_tests.yaml
file using a matrix build specification(instead of having .travis.yml and .azure-pipelines.yml). Will keep Travis CI because it's needed for the PyPI releases, but will disable Azure Pipelines for now.Original attempt was made in weiji14#88, see some of the previous discussion/reviews there.
Fixes #
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.