153-End to End build test. #266
Conversation
…ad of no args - much easier to test
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #266 +/- ##
==========================================
+ Coverage 97.90% 98.61% +0.71%
==========================================
Files 11 11
Lines 811 1010 +199
==========================================
+ Hits 794 996 +202
+ Misses 17 14 -3 ☔ View full report in Codecov by Sentry. |
src/access_nri_intake/cli.py
Outdated
| @@ -87,18 +94,19 @@ def _check_build_args(args_list): | |||
|
|
|||
| if len(names) != len(set(names)): | |||
| seen = set() | |||
| dupes = [name for name in names if name in seen or seen.add(name)] | |||
| dupes = [name for name in names if name in seen or seen.add(name)] # type: ignore | |||
| # seen.add(name) returns None & so is always Falsey - so what is it doing? | |||
There was a problem hiding this comment.
Please tell me Falsey is a British programming idiom :P
There was a problem hiding this comment.
Seriously though, I think the or is being used as a sneaky way to avoid an if else statement to add things to the "I've now seen this name" set (seen).
It's going in order through the names list. The or is evaluated left to right, so:
- If the name has been seen earlier in the list (i.e.,
name in seen == True), then it gets added todupes. Theseen.add(name)is never reached. - If
name in seen == False, then it moves on to the second part of theorstatement, which addsnameto theseenset. You're right that it's always False, but that's the point - it'sFalseon this pass, but now that it's been added to theseenset, if the same name recurs on a future pass, it is now known to be a duplicate.
(Interestingly, False or None evaluates to None, not False, but for this list comprehension I'm assuming anything not True is close enough to False to not care.)
In [13]: (False or None) is None
Out[13]: True
In [14]: None or False
Out[14]: False
There was a problem hiding this comment.
Also, if a duplicated is repeated N times, then it will appear (N-1) times in the error message below - not sure if that's a bug or feature.
There was a problem hiding this comment.
Funnily enough, I think Falsey is pretty common worldwide - just googled it and it looks like it mostly crops up in javascript land though.
More importantly though, I had completely forgotten that the or will get executed left -> right & that seen.add() will still evaluate, even though it returns None.
I think the False or None == None and None or False == False thing is the result of the same trick - None and False are both falsey, so we wind up getting the second value in either case. Makes perfect sense, but completely counterintuitive.
Either way, I'll remove the unnecessary comment & see if I can strip it down to 1 error message, not N-1.
e2e/conftest.py
Outdated
| Get the XFAILS environment variable. We use a default of 1, indicating we expect | ||
| to add xfail marker to `test_parse_access_ncfile[AccessOm2Builder-access-om2/output000/ocean/ocean_grid.nc-expected0-True]` | ||
| unless specified. |
There was a problem hiding this comment.
Probably should be updated to reference which test a value of 1 will point to.
There was a problem hiding this comment.
Good spot
e2e/conftest.py
Outdated
| """ | ||
| This function is called by pytest to modify the items collected during test | ||
| collection. We use it here to mark the xfail tests in | ||
| test_builders::test_parse_access_ncfile when we check the file contents & to |
e2e/conftest.py
Outdated
| if ( | ||
| item.name | ||
| in ( | ||
| "test_parse_access_ncfile[AccessOm2Builder-access-om2/output000/ocean/ocean_grid.nc-expected0-True]", |
e2e/test_end_to_end.py
Outdated
| Return the current catalog as an intake catalog. | ||
| """ | ||
| metacat = intake.cat.access_nri | ||
| yield metacat |
There was a problem hiding this comment.
What's the idea behind yielding here instead of returning? (Not an issue, just for my own edification.)
There was a problem hiding this comment.
To be honest, I'm not entirely sure. Pytest recommend using yield for fixtures, in particular if you might want some teardown code.
I originally wrote this using the tempfile module - I'd forgotten pytest contains its own temporary filesystem handlers - so I thought I might have needed to include some teardown code.
Given there is no teardown, I think it makes no practical difference.
|
A couple of other things:
|
|
Regarding the Gadi deployment actions, the workflow file can be found here. This workflow triggers the I’ve deployed the current main branch to the conda/access-med-0.10 environment. Fingers crossed, it works as expected! |
|
Just tested with a subset that only includes CMIP5 and it worked fine: |
I virtually always run pytest using I think both your suggestions would make this much cleaner. |
|
I've done a bit more looking to this - frustratingly, looks like there might be some complications into the pytest |
… working smoothly from there
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
.github/workflows/e2e.yaml
Outdated
| steps: | ||
| - name: Checkout repository | ||
| ### Latest at time of writing | ||
| uses: actions/checkout@v4.2.2 |
There was a problem hiding this comment.
I would just assume we have a copy on Gadi for now. You can delete the checkout and the rsync.
Closes #153.
This contains an end to end build test, where a subset of the catalog is built, and queries executed against it.
clip.pymodule to make testing easier.@rbeucher can you link me to the Gadi SSH workflows you showed me the other day & I'll create a trigger for this based off those.