Fix issue3296: prevent writing when running tests in CDMS#3297
Conversation
bsipocz
left a comment
There was a problem hiding this comment.
Actually, as this is a new kwarg, it will need a changelog entry.
And maybe consider being a bit more verbose with naming the kwarg, but it's not a blocker.
| use_cached : bool, optional | ||
| If True, use the cached file if it exists. If False, download the | ||
| file from the CDMS server and save it to the cache (if ``write`` is set). | ||
| write : bool, optional |
There was a problem hiding this comment.
A more verbose arg would be nice, maybe call this write_cached or refresh_cashed?
There was a problem hiding this comment.
I'll be more verbose, but I think a different keyword is warranted... write_new_cache, probably.
There was a problem hiding this comment.
Sure. Anything more that just write is an improvement, and write_new_cache is quite good. Though maybe it will need a switch in logic, e.g. if it's set then ignore whatever is in use_cache rather than the other way around.
…n, and add changelog entry
bsipocz
left a comment
There was a problem hiding this comment.
Weird that the CI wasn't kicking in, I'm fairly certain one of the lines is too long. I also now see uncatched warnings, please address those and then we can merge this.
|
|
||
| if use_cached: | ||
| if write_new_species_cache: | ||
| warnings.warn("use_cached and write_new_species_cache are both set to True; write_new_species_cache will be ignored. If you meant to update the cache, set use_cached to False.") |
There was a problem hiding this comment.
This now issues a couple of warnings in the tests that we need to handle.
There was a problem hiding this comment.
Uhoh.... that shouldn't be happening.
…urity' to prevent overwriting, but that's not needed
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3297 +/- ##
==========================================
+ Coverage 69.36% 69.47% +0.10%
==========================================
Files 232 232
Lines 19692 19707 +15
==========================================
+ Hits 13659 13691 +32
+ Misses 6033 6016 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| linelists.cdms | ||
| ^^^^^^^^^^^^^^ | ||
|
|
||
| - Add a keyword to control writing of new species cache files. This is needed to prevent tests from overwriting those files. [#3300] |
There was a problem hiding this comment.
You're living in the future :) (But I'll fix it at release time :) )
There was a problem hiding this comment.
oh shoot, I had fixed it and just didn't push yet... I got some failing tests still to fix
There was a problem hiding this comment.
I did run the tests prior merging and it was all clear (besides a ConnectionError, but I double checked and that one was present on main, too.
There was a problem hiding this comment.
I think I needed to update my local table....
Anyway, I have a fix to push because this last test took too long to run for my liking
There was a problem hiding this comment.
I would say don't stress on the retrieval one as we don't run it often and it's only 2s on my local. But if you would fix the really long running (400+s )and then failing one, the test_regression_allcats, that would be great.
|
Thanks! |
|
There might be a related failing test - I'm not sure yet. I got this error, but it coincided with a server error: I'm re-running. |
Simple fix for #3296. We do want the remote tests grabbing stuff from the remote server, but you're right, we don't want it modifying data files in the repository.