Adapt tests to new version of cf-units

[zklaus]

Description

This is a bugfix to address recent test failures due to a change in cf-units.

Closes #1655

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.

• 🧪 The new functionality is relevant and scientifically sound
• 🛠 This pull request has a descriptive title and labels
• 🛠 Code is written according to the code quality guidelines
• 🧪 and 🛠 Documentation is available
• 🛠 Unit tests have been added
• 🛠 Changes are backward compatible
• 🛠 Any changed dependencies have been added or removed correctly
• 🛠 The list of authors is up to date
• 🛠 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

[zklaus]

This is a start to address recent test failures for the core similar to those in the tool that arise from the changes in cf-units.

I need help with two things: How to convert the unittest.TestCase derived classes in the cmip5 fix tests, and how to deal with the inevitable regression. @bouweandela or @schlunma, any ideas?

[schlunma]

Why do you want to get rid of unittest.Testcase? As an alternative for setup and teardown I would use fixtures.

[zklaus]

The removal of unittest was motivated by the fact that the self.assertEquals has to be changed to some form of test with the in operator. It's possible that there is a self.assertIn and just using self.assert would be an alternative. However, that would give poorer output and if I recall correctly, we discussed getting rid of unittest altogether recently in an issue, though I thought why not give it a try. Fixtures are certainly the way to go; I should have been more precise in my question, which really was: Do you know off the top of your head how to use fixtures in classes?

[schlunma]

Ah I see.

I think you could just define the fixture outside of the class and then use it inside the class like you would normally do. In addition you could add a scope='class' parameter to the fixture, but I don't think that's useful here. To mimic the behavior of setUp the default scope (fixture is destroyed after every test) should be the one to go.

[zklaus]

Alright, I'm stepping off here. Release managers, feel free to continue with this branch or to start your own fixes.

[sloosvel]

Is this worth another release candidate?

[zklaus]

I would say so, yes. But it's your call. I am also not fully aware of other changes that may have happened since the last release candidate, but generally speaking, the release is expected to be identical to the last release candidate.

[sloosvel]

Alright, I'm stepping off here. Release managers, feel free to continue with this branch or to start your own fixes.

And what is is that is left to be finished in here? It's quite surprising to see these kind of answers considering the kind of comments that I usually have to hear about my own "unfinished work".

[valeriupredoi]

I think this needs to be included in the RC rather than straight into the release since all this calendars business is a source of major pain in the rear and can lead to some unwanted changes in the actual diagnostics' results. @zklaus would you have some time to polish this a bit more? If not I can take over from here, no problemo 🍺

[sloosvel]

No worries, I got a new branch to work on this. I will close this one.

[bouweandela]

For the upcoming relaese and to avoid a lot of last minute work, would it be an option to just pin cf-units to the version that we know works and that @sloosvel already ran all the tests with?

[valeriupredoi]

@bouweandela I think that's a very good idea indeed!

[zklaus]

That could be an option, though note that the corresponding fix in ESMValTool is already in (ESMValGroup/ESMValTool#2707) and that this only changes the tests. But it is better to pin anyway since there is a chance that this changes the calendar in some intermediate file that could throw off some diagnostic. The fixes here are written to support both versions.

[zklaus]

Alright, I'm stepping off here. Release managers, feel free to continue with this branch or to start your own fixes.

And what is is that is left to be finished in here? It's quite surprising to see these kind of answers considering the kind of comments that I usually have to hear about my own "unfinished work".

It's the responsibility of the release managers to take care of the nightly tests. They have been broken for a week now, so I did the digging for the cause and how to fix it. I'm sorry if this came across as anything but helpful.

[sloosvel]

It's the responsibility of the release managers to take care of the nightly tests. They have been broken for a week now, so I did the digging for the cause and how to fix it. I'm sorry if this came across as anything but helpful.

I know, but I had to finish running recipes for a release candidate that could have been avoided. In any case, everyone in favour of pinning then?

[valeriupredoi]

@sloosvel am pinning it as we speak 🍺

[valeriupredoi]

about the tests - we should think of a stricter policy while we're working on a RC or stable release, something like "thou shall not pass" if the tests fail

[zklaus]

@valeriupredoi, I am not sure what you mean, but perhaps that's a new issue/discussion?

[valeriupredoi]

@valeriupredoi, I am not sure what you mean, but perhaps that's a new issue/discussion?

what I mean is to have a clause in the push to pypi that all GA tests have passed - ok maybe too strict - have the tests open an issue automatically if one test fails, and maybe turn that feature on only during release periods so we don't overkill?

[bouweandela]

have the tests open an issue automatically if one test fails

I like that, there is probably a github action for that

[valeriupredoi]

@bouweandela there's a github action even for when you forget to turn on that github action 🤣