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

Run Continuous Integration tests on Github Actions #475

Merged
merged 41 commits into from
Jul 10, 2020
Merged

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented Jun 10, 2020

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

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

weiji14 and others added 10 commits June 5, 2020 15:04
Signed-off-by: Wei Ji <weiji.leong@vuw.ac.nz>
Signed-off-by: Wei Ji <weiji.leong@vuw.ac.nz>
Signed-off-by: Wei Ji <weiji.leong@vuw.ac.nz>
Signed-off-by: Wei Ji <weiji.leong@vuw.ac.nz>
Signed-off-by: Wei Ji <weiji.leong@vuw.ac.nz>
Try just caching ~/.gmt/cache and ~/.gmt/server.
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
@weiji14 weiji14 added the maintenance Boring but important stuff for the core devs label Jun 10, 2020
@weiji14
Copy link
Member Author

weiji14 commented Jun 10, 2020

Good work on the wget workaround @seisman! Tests seem to be failing on the new earth_relief grids only, should fix that first.

@seisman
Copy link
Member

seisman commented Jun 10, 2020

Good work on the wget workaround @seisman! Tests seem to be failing on the new earth_relief grids only, should fix that first.

FYI, we can't use curl to download files on Linux and windows, as it always timeouts. It's not clear to me why wget works and curl doesn't. GMT relies on the curl library to download remote files with a 10-s timeout, and of course GMT fails just like the curl command.

@weiji14
Copy link
Member Author

weiji14 commented Jul 9, 2020

I think we shouldn't cache conda packages, to make the actions more readable.

Sounds good. We can always add it back in later if we decide a cache for conda packages is needed.

@seisman
Copy link
Member

seisman commented Jul 9, 2020

I think we shouldn't cache conda packages, to make the actions more readable.

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.

.github/workflows/ci_tests.yaml Outdated Show resolved Hide resolved
Comment on lines +71 to +77
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
Copy link
Member

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
Copy link
Member

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.

.github/workflows/ci_tests.yaml Show resolved Hide resolved
@seisman seisman marked this pull request as ready for review July 10, 2020 01:22
Copy link
Member

@seisman seisman left a 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}
Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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!

Copy link
Member Author

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.

Copy link
Member

@seisman seisman Jul 10, 2020

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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

Copy link
Member Author

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.

@weiji14
Copy link
Member Author

weiji14 commented Jul 10, 2020

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 🎉

@weiji14 weiji14 merged commit 158ef09 into master Jul 10, 2020
@weiji14 weiji14 deleted the gh-actions-ci branch July 10, 2020 02:26
@seisman seisman added this to the 0.2.x milestone Jul 10, 2020
@seisman seisman added this to In progress in Release v0.2.x via automation Jul 10, 2020
@seisman seisman moved this from In progress to Done in Release v0.2.x Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants