Skip to content
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

combine and skip cirrus-ci tasks #4179

Closed
wants to merge 2 commits into from

Conversation

bjlittle
Copy link
Member

@bjlittle bjlittle commented Jun 4, 2021

🚀 Pull Request

Description

This PR rationalises the cirris-ci tasks.

We require to be a little bit more savvy when it comes to the resources that we are using on cirrus-ci.

As such this PR skips the minimal tests, and combines the doc-tests and doc gallery tests into one task.

The effort and resource spent on performing the minimal tests are duplicated by the full tests, therefore let's skip them on cirrus-ci. Note that, minimal tests can still be run locally via nox, if the developer wishes.

Also, there is effort and resource spin-up duplication between the documentation tasks, therefore the doc-tests and gallery tests have been combined at the nox level - akin to why the linting task combines several similar jobs together.

Note that, the documentation link-check task will soon be combined with the new spell checking task once #3882 lands.


Consult Iris pull request check list

@rcomer
Copy link
Member

rcomer commented Jun 5, 2021

If we don’t routinely run the minimal tests, we risk breaking them. For example at #4136 I initially failed to add the skip_data decorator to the new test class, and only realised my mistake when the cirrus minimal tests fell over. I have no opinion on whether the minimal tests need to remain functional though.

@bjlittle
Copy link
Member Author

bjlittle commented Jun 5, 2021

@rcomer A great point. Perhaps enabling a crobjob on cirrus-ci once a week might be a happy compromise here... I'll have a play.

@rcomer
Copy link
Member

rcomer commented Jun 6, 2021

Thinking about it, I'm actually not sure what the benefit of the minimal tests is. It's certainly never occurred to me to try to run them locally, and I don't think they're advertised in the dev guide either. In many cases it seems a bit arbitrary whether data is loaded from file or just some dummy data created instead. So I'm not sure what circumstances you would use them.

@trexfeathers
Copy link
Contributor

Thinking about it, I'm actually not sure what the benefit of the minimal tests is.

The only benefit I've been aware of is a faster development cadence when it gives faster notification that I've pushed a mistake to my PR. That's less relevant now we have faster and more parallel CI.

I'm in favour of removing them entirely. I agree with @rcomer that they should be run [semi-]regularly if they continue to exist.

@bjlittle
Copy link
Member Author

bjlittle commented Jun 7, 2021

Thinking about it, I'm actually not sure what the benefit of the minimal tests is. It's certainly never occurred to me to try to run them locally, and I don't think they're advertised in the dev guide either. In many cases it seems a bit arbitrary whether data is loaded from file or just some dummy data created instead. So I'm not sure what circumstances you would use them.

@rcomer I can't disagree with you.

We could advocate that if you want to run the tests then you need the data, otherwise no tests will run.

Historically, I think the original purpose behind the minimal tests were that they were a cheap and fast suite of tests to run instead of all the tests... but they are actually rather bloated now and almost as expensive to run and the full suite of tests.

I'd love to restructure the tests (and it's pretty high on my hit list of things to do) but if there is consensus I'd happily drop the concept of minimal tests.

I can appreciate that they do provide the convenience to run the tests without configuring the data... but is that a good enough reason to justify the difference?

Do any @SciTools/iris-devs have an opinion?

@trexfeathers
Copy link
Contributor

I can appreciate that they do provide the convenience to run the tests without configuring the data... but is that a good enough reason to justify the difference?

Definitely not enough justification alone, IMO.

@rcomer
Copy link
Member

rcomer commented Jun 7, 2021

they do provide the convenience to run the tests without configuring the data

but the full tests need running eventually to check you haven't broken anything. If we had only used the test data files for testing functions that do loading, I could safely say "I'm not changing file I/O, so don't need the test data". But we are very much not in that situation!

It also occurs to me that 10 years ago we probably had a different view on what constitutes a large volume of data. So might have been more worried than we are now about asking people to download it?

@bjlittle bjlittle closed this Jun 23, 2021
@bjlittle bjlittle deleted the cirrus-ci-combine branch June 23, 2021 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants