-
Notifications
You must be signed in to change notification settings - Fork 12
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 pytest on macos in GH actions #262
Conversation
49be046
to
7cb7200
Compare
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/262/index.html |
a541c70
to
9c75572
Compare
774a192
to
e24b02e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #262 +/- ##
=======================================
Coverage 95.35% 95.35%
=======================================
Files 171 171
Lines 36801 36805 +4
=======================================
+ Hits 35090 35094 +4
Misses 1711 1711
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
8990cee
to
68ee1be
Compare
5e6ca02
to
2b37aed
Compare
f947673
to
623ba41
Compare
I think this is ready for a look. I had to do a few things in addition to ensure compatibility with MacOS, but I think it generally helps to increase being platform-agnostic. In detail:
|
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.
Looks good to me, and very useful in general. Thanks for grinding this one out! 🙏
Only question I have a is about the tuple-comparison changes, but otherwise this looks great. GTG from me once that is answered.
@@ -249,7 +249,7 @@ def test_file_item2(testdir): | |||
) | |||
|
|||
# Files don't have dependencies (direct dependencies, anyway) | |||
assert item.dependencies is () | |||
assert isinstance(item.dependencies, tuple) and not item.dependencies |
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'd be curious to know why this is necessary?
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.
Pytest doesn't like is
comparison against literals:
/home/runner/work/loki/loki/loki/batch/tests/test_batch.py:252: SyntaxWarning: "is" with a literal. Did you mean "=="?
assert item.dependencies is ()
See for example here: https://github.com/ecmwf-ifs/loki/actions/runs/10124810197/job/27999587354#step:8:120
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.
Ok, good to know. Thanks.
With people using Loki locally on their MacBooks, it seems useful to test this as part of the CI.