Add a more friendly and useful message when using default config file #1233
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1233 +/- ##
==========================================
+ Coverage 85.54% 86.37% +0.83%
==========================================
Files 188 188
Lines 9150 9153 +3
==========================================
+ Hits 7827 7906 +79
+ Misses 1323 1247 -76
Continue to review full report at Codecov.
|
|
OMG OMG the test coverage decreases by 0.01% 😮 🤣 I like this functionality a lot but is there any way we can set thresholds above which we accept the codecov test to pass? @bouweandela |
|
Why not just improve the coverage? |
|
Nice touch V! I changed it a bit so we keep the actual path to the config file in the config dictionary, so we can log it directly. I think is clearer that way and we also do not have to modify the message in the (unlikely) case that we modify the default path |
|
awesome, cheers Javi! Klaus, I refuse to write tests for print statements, just kidding, I should actually 😁 |
|
I mean, the print statement is probably part of a function that does something. As such, it should be covered by tests because that function is covered by tests. |
|
@jvegasbsc I've added a (rather peasant-y) test that actually calls and runs |
|
@bouweandela we need to fix this Codecov thingie, man, the latest code changes improve test coverage for |
|
@jvegasbsc careful that now there are two R test diags that fail in |
|
I have changed the run test to do not overwrite the default config user and use one in the tmp path and added a new test for the lines that codecov is claiming that now are not covered. @valeriupredoi, probably you should move the codecov config changes to its own pull request |
|
I messed a bit too much with this PR to be an independent reviewer. @sloosvel, can you take a look? |
|
cheers @jvegasbsc - I took out the codecov changes to the config and cleaned up your bad key that was making the diag test run fail 😁 Heads up that the errors reported by |
We finally got quite an improvement |
yesh we did - it is biutiful 😁 |
|
cheers muchly @sloosvel - @jvegasbsc if you don't want to add more then you can just merge it, mate, I would but that'd be a bit biased 😁 |
jvegreg
changed the title
|
The conda build is failing due to timeout, but that's not our fault (see #1243) |
| log_dir = './esmvaltool_output' | ||
| log_file = os.path.join(log_dir, | ||
| os.listdir(log_dir)[0], 'run', 'main_log.txt') | ||
| os.system("rm -r esmvaltool_output") |
There was a problem hiding this comment.
This looks very unsafe, why not use tmp_path for the output and let pytest clean up?
| # default: ~/default_inputpath | ||
| # CORDEX: ~/default_inputpath | ||
| # Comment out these when using a site-specific path | ||
| rootpath: |
There was a problem hiding this comment.
This was commented out because we got the complaint from users that these default settings do not work ever for anyone.
Description
When there is no user-specified config-user file, the info stdout says Using config file None, which is both untrue and user confusing, even though the actual value is indeed
NoneCloses ESMValGroup/ESMValTool#2235
Link to documentation:
Before you get started
Checklist
It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.
To help with the number pull requests: