Remove profile_diagnostic from diagnostic settings and increase test coverage of _task.py

[valeriupredoi]

Description

Closes #1392

Link to documentation:

---

Before you get started

• ☝ Create an issue to discuss what you are going to do

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.

• 🛠 This pull request has a descriptive title and labels
• 🛠 Changes are backward compatible
• 🛠 All checks below this pull request were successful

---

To help with the number pull requests:

• 🙏 We kindly ask you to review two other open pull requests in this repository

[codecov]

Codecov Report

Merging #1404 (5d3e5a3) into main (6ae6020) will increase coverage by 0.84%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1404      +/-   ##
==========================================
+ Coverage   89.85%   90.70%   +0.84%     
==========================================
  Files         197      197              
  Lines       10489    10499      +10     
==========================================
+ Hits         9425     9523      +98     
+ Misses       1064      976      -88     

Impacted Files	Coverage Δ
esmvalcore/_task.py	83.82% <100.00%> (+19.22%)	⬆️
esmvalcore/_recipe.py	95.78% <0.00%> (+0.10%)	⬆️
esmvalcore/_main.py	77.35% <0.00%> (+2.13%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ae6020...5d3e5a3. Read the comment docs.

[bouweandela]

It looks like profile_diagnostic will still be written to the NCL interface file. Could you have a look?

If you increase the test coverage I'll buy you a 🍺 next time we meet 😉

[valeriupredoi]

yesh man, on it today! I'll have a look at the NCL hiccup too 👍

[valeriupredoi]

OK @bouweandela I fixed the NCL bit, and now we are testing for profile_diagnostic not being a dig setting, note however that I also turned back on the diagnostic test on Python - why did we comment that out?

[valeriupredoi]

@bouweandela I don't understand why codecov is complaining - those lines are nice and green (checked for) in my copy of the tests coverage (local run) - apart from that, this is RFR 🍺

[valeriupredoi]

agh nevermind! I figured it out - we're running a very low number of task tests in CircleCI - that's not good - coverage for _task.py goes down from 84% to 67% the way we run the tests with "no installation" flag - I removed it, it should be fine, it's not slowing the tests down visibly

[valeriupredoi]

OK am converting this to a Draft until I get to the bottom of the issue of running a Python diagnostic in a test env on Circle - that's a bit disconcerting at the mo, we should be able to test the full top to bottom task infrastructure via a Py diag

[schlunma]

This fails with a

KeyError: 'diag_script_info'

now.

main_log_debug.txt

@valeriupredoi I agree with @bouweandela here that it might make sense remove your changes here to files that the tests that are not relevant for this PR. You think we can include this in the first release candidate (tomorrow morning)?

[valeriupredoi]

This fails with a

KeyError: 'diag_script_info'

now.

main_log_debug.txt

@valeriupredoi I agree with @bouweandela here that it might make sense remove your changes here to files that the tests that are not relevant for this PR. You think we can include this in the first release candidate (tomorrow morning)?

no, the tests are very much relevant to the second main scope of this PR - first is to get rid of that profile settings arg, and second is to increase the coverage for tasking which was poor previously. After a relatively major kerfuffle with git (I was sound asleep when I merged only God knows what I merged in here, took me an hour to fix it) I finally fixed the test to be more secure than previously (and fixed the issue with the NCL setting key too, cheers Manu!). If you don't run the Python mock test you don't test the tasking proper, and Bouwe made sure that he wrote the test in a rather "scratch your left ear with your right hand" way, so my hand were tied and couldn't go around it too much - why do you need a result.yml file in the main output dir is beyond me 😁 Anyway, am done now, take it or leave it 😆

[schlunma]

Makes sense, thanks V.! I will have a another look at this once #1480 is fixed 👍

[valeriupredoi]

thanky much, Manu! 🍺

[schlunma]

Could you also have a look at codecov? Cheers! 🍻

[valeriupredoi]

yes, I knew it'll complain - the bugger is now complaining that the increase in coverage dropped from 19% to 18% 🤣 But that's an extra 18%, ya codecov dummer! The problem is that I would have to write another test case for when the profile_diagnostic is not inside the diag_settings_info dict, but I don't think running yet another mock diagnostic would justify testing only for those two lines only - what you think?

[schlunma]

It's not complaining about the drop in the increase, but that not all new lines are hit. You need a test case that satisfies if 'diag_script_info' not in settings_copy:; you could probably just copy the test you added to tests/unit/task/test_diagnostic_task.py with the entry for diag_script_info.

[valeriupredoi]

cheers, Manu! That's only for the NCL case and it's not immediately straightforward to me how to do that without running another mock diagnostic (which I don't see the need for, it just slows down the test) - any ideas? You speak NCL much better than me 😁

[schlunma]

You don't need to run a mock diag for that. I think you can really just copy the test you added to test_diagnostic_task.py and modify it to something like:

def test_write_ncl_settings(tmp_path):
    """Test minimally write_ncl_settings()."""
    settings = {
        'run_dir': str(tmp_path / 'run_dir'),
        'profile_diagnostic': False,
        'var_name': 'tas',
    }
    file_name = tmp_path / "settings"
    write_ncl_settings(settings, file_name)
    with open(file_name, 'r') as file:
        lines = file.readlines()
        assert 'var_name = "tas"\n' in lines
        assert 'profile_diagnostic' not in lines

[valeriupredoi]

Manu you're a genius! I totally forgot about the unit test, which was indeed changed by me too, even in this PR 😆 Maybe I should call it quits today 🤦‍♂️

[schlunma]

Haha, please don't quit just yet, I need your review for a fix for #1480 in a couple of minutes 😄

[valeriupredoi]

am still in, dude 🖖

[schlunma]

Just tested everything again, works fine now! Thanks V!!

[valeriupredoi]

thoughts and prayers for my hero Manu 🇺🇸 🦅 Cheers much dude!!