-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add test to latest and oldest supported aiida-core #558
Add test to latest and oldest supported aiida-core #558
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #558 +/- ##
=======================================
Coverage 87.07% 87.07%
=======================================
Files 27 27
Lines 4649 4649
=======================================
Hits 4048 4048
Misses 601 601
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
48cac20
to
b2c6345
Compare
b2c6345
to
c4375a7
Compare
The unittest with python==3.9 and aiida-core==2.1.2 failed because of PyYAML issue yaml/pyyaml#723 So in fact the oldest support version is I think we can make the oldest support aiida-core version to be |
Ugh, that's annoying. I'll take a look, thanks! |
I checked on Dockerhub and we released an image with aiida 2.2 10 months ago. So let's just test with this version and not waste too much time on this. Can always improve this in the future if needed. |
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.
Tests are passing now. 🎉 Very happy with this change, will make us more confident to not break backwards compatibility. Thanks @unkcpz!
Couple small fixes but otherwise good to go.
tests_notebooks/docker-compose.yml
Outdated
# TODO: after the oldest aiida-core supported version is v2.4.0, | ||
# move to use ghcr.io registry to pull the image, the dockerhub registry has | ||
# a rate limit of 100 pulls per 6 hours. | ||
# aiida-core < 2.4.0 is not push to ghcr.io because of CI issues | ||
# The CI is fixed in https://github.com/aiidalab/aiidalab-docker-stack/pull/390 and | ||
# from aiida-core 2.4.0 the images are pushed to ghcr.io. | ||
image: aiidalab/full-stack:${TAG:-latest} |
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.
Thank you for the comment, very good for future reference
# TODO: after the oldest aiida-core supported version is v2.4.0, | |
# move to use ghcr.io registry to pull the image, the dockerhub registry has | |
# a rate limit of 100 pulls per 6 hours. | |
# aiida-core < 2.4.0 is not push to ghcr.io because of CI issues | |
# The CI is fixed in https://github.com/aiidalab/aiidalab-docker-stack/pull/390 and | |
# from aiida-core 2.4.0 the images are pushed to ghcr.io. | |
image: aiidalab/full-stack:${TAG:-latest} | |
# TODO: When we drop support of images with aiida-core<2.4.0, we should go back to using | |
# the ghcr.io registry to pull the image, | |
# since the dockerhub registry has a rate limit of 100 pulls per 6 hours. | |
# Images with aiida-core<2.4.0 were not pushed to ghcr.io because of CI issues, | |
# which were fixed in https://github.com/aiidalab/aiidalab-docker-stack/pull/390 | |
image: aiidalab/full-stack:${TAG:-latest} |
.github/workflows/ci.yml
Outdated
@@ -109,6 +109,7 @@ jobs: | |||
matrix: | |||
tag: [latest] |
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.
tag: [latest] |
The tag variable is not used in the test-package1
workflow.
browser: [Chrome, Firefox] | ||
python-version: ['3.10'] | ||
aiida-core-version: [2.1.2, 2.4.3] # test on the latest and the oldest supported version |
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.
Here version 2.1.2
actually works since it is pre-installed in the image. So we have at least some coverage.
Thanks for the review @danielhollas |
No description provided.