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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add required functional parameters for extract time in recipe_er5.yml #1978

Merged
merged 1 commit into from Jan 13, 2021

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Jan 13, 2021

The lack of these, tested against the latest development master of esmvalcore, make tests/integration/test_recipes_loading.py fail with

E               ValueError: Missing required argument(s) {'start_year', 'start_month', 'start_day'} for preprocessor function extract_time

Two points here:

  • please run the tests after the last commit to a branch, and have a look at the Github Actions tests too, even though those are run with a released esmvalcore
  • specifically to @bouweandela I am starting to have it up to here with the cached tests that lag a lot behind the most up to date version of the development master's mate, we find broken tests that pass no problemo with the cached code since it's old; I know that this is good for faster testing but I am starting to think we need a full time job for someone to test and fix newly broken tests and that ain't gonna be me 馃榿 馃憤

@Peter9192
Copy link
Contributor

Good find! I'm surprised how this has gone unnoticed. And I see your point about the tests man, the test log in this commit is also quite overwhelming...

@Peter9192
Copy link
Contributor

Would it make sense to add a CI workflow to ESMValCore that runs some tests for ESMValTool so you can spot downstream problems early on?

Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Vi!
All remaining failing tests seem to be related to #1980, so I'm happy to see this get merged.

@valeriupredoi
Copy link
Contributor Author

No worries, happens to all of us 馃槃
the test tests fail coz of this #1980 - again, coz the cached code is old.

Would it make sense to add a CI workflow to ESMValCore that runs some tests for ESMValTool so you can spot downstream problems early on?

might be a good idea but if the CI runs a purely development test that checks out the latest ESMValCore master that'd do for us - but Bouwe will say that's just too slow. Personally I'd rather have that as a cron job, so we can have a look at it in the morning. If not, I'll plug it in Github Actions

@valeriupredoi valeriupredoi merged commit 33c1883 into master Jan 13, 2021
5 of 6 checks passed
@valeriupredoi valeriupredoi deleted the fix_recipe_era5 branch January 13, 2021 13:57
@bouweandela
Copy link
Member

To add to the discussion here: the ESMValTool repository works with the latest released version of ESMValCore, to provide stability for diagnostic developers. Previously we regularly had people complaining that they were developing their diagnostics against a moving target.

I am starting to have it up to here with the cached tests

@valeriupredoi The tests that are fixed in this pull request were broken in the development version of ESMValCore when ESMValGroup/ESMValCore#796 was merged, which is not in a released version of the ESMValCore yet. Therefore this is not related to any caching.

To fix this kind of minor problems, we have one week between the ESMValCore release and the ESMValTool feature freeze in the release schedule. It's not perfect, because during that one week the development version of ESMValTool may not work quite as expected, but at the moment I don't know any better way of doing this, so if anyone has suggestions?

Would it make sense to add a CI workflow to ESMValCore that runs some tests for ESMValTool so you can spot downstream problems early on?

Personally I'd rather have that as a cron job, so we can have a look at it in the morning. If not, I'll plug it in Github Actions

@valeriupredoi @Peter9192 This is definitely not something we would want to run on every commit, but running it nightly might indeed provide some early warning. The job will fail a lot though, so the temptation to stop looking at it after you see it failing for the 20th time in a row might be quite big.

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.

None yet

3 participants