-
-
Notifications
You must be signed in to change notification settings - Fork 413
Fix issue3296: prevent writing when running tests in CDMS #3297
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
Conversation
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.
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.
astroquery/linelists/cdms/core.py
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more verbose arg would be nice, maybe call this write_cached
or refresh_cashed
?
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'll be more verbose, but I think a different keyword is warranted... write_new_cache
, probably.
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.
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
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.
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.
astroquery/linelists/cdms/core.py
Outdated
@@ -317,6 +327,8 @@ def get_species_table(self, *, catfile='partfunc.cat', use_cached=True, | |||
""" | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now issues a couple of warnings in the tests that we need to handle.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're living in the future :) (But I'll fix it at release time :) )
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.