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

Fix recipe loading tests for esmvalcore before and after version 2.8 #3020

Merged
merged 13 commits into from Jan 25, 2023

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Jan 19, 2023

Description

esmvalcore's _recipe.py has become a submodule in its own right in esmvalcore>=2.8 and we should account for that when we test the recipe loading and interaction with esmvalcore in tests/integration/test_recipes_loading.py


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.

@valeriupredoi valeriupredoi added this to the v2.8.0 milestone Jan 19, 2023
@valeriupredoi
Copy link
Contributor Author

@ESMValGroup/technical-lead-development-team anyone's got time to have a look at this pls, it bugs me we have red tests 😁

Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

Thanks V! The code looks good to me. I understand that this distinction between v2.7 and v2.8 accounts for all the different tests. Shall we just open an issue to keep in mind to remove this distinction when no longer needed? (Shortly before or after the next release?)

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Jan 24, 2023

cheers muchly @remi-kazeroni 🍺 The problem is the tests will fail if we remove this and we get users that want to use an older version and run the tests after installation - I'd keep it there until we don't support 2.7 anymore (may be a while) - forgot to add, this would creep in only when installing in full development mode, rather than Core as a conda dependency since that will be pinned >=2.8 after the 2.8 release

@schlunma
Copy link
Contributor

Two comments before merging:

  1. I think using something like packaging.version.parse might be simpler and clearer to check for the version:
if version.parse(core_ver) < version.parse('2.8.0'):
    ...
  1. Regarding @remi-kazeroni's comment: Once ESMValTool 2.8 is released, it's impossible to use ESMValCore < 2.8 with it (because of a pin similar to that) I guess. On the other hand, ESMValTool < 2.8 cannot be used with ESMValCore >= 2.8 (because of the same pin). So this if is only really necessary at the moment where one can have a development installation of ESMValTool 2.8.0.dev with ESMValTool v2.7.x. So I agree with @remi-kazeroni that we should remove the if right after the next release.

@valeriupredoi
Copy link
Contributor Author

@schlunma cheers! ah yes - I tried rack my brains what the package was called - I had completely forgotten its name, yeah good point - let me change with that. Regarding point 2 - you guys keep forgetting about the pure development install: one installs ESMValTool from source (Core 2.8 is picked up from tagged/deployed), then they decide to grab the Core gitball at a version 2.7 or lower (because reasons, who knows what bozo would do that?) then install it in dev mode, and then they would want to run the tests (again, coz they're strange people hahah) - fail, that's why this thing here for longer than now

@valeriupredoi
Copy link
Contributor Author

@schlunma all done now, cheers for the packaging suggestion! If you could have a last look and merge, that'd be fab, bud 🍺

@schlunma
Copy link
Contributor

Right - I thought we also have that same pin in setup.py, but apparently we don't. Then it's probably safer to use that, though I think that there will probably many other places where using an old core will fail πŸ˜„

Regarding the check: Do you have an idea why if version.parse(core_ver) < version.parse('2.8.0'): did not work? This would be the better option, just in case we do a v2.7.2 πŸ˜‚

@valeriupredoi
Copy link
Contributor Author

Regarding the check: Do you have an idea why if version.parse(core_ver) < version.parse('2.8.0'): did not work? This would be the better option, just in case we do a v2.7.2

You do a 2.7.2, I won't 🀣 The parser seems to be semantic ie a 2.8.0.dev is indeed smaller than a 2.8.0 and that made tests fail, and they'd keep failing until we actually release 2.8.0

@valeriupredoi
Copy link
Contributor Author

though I think that there will probably many other places where using an old core will fail

That, sir, is very true - but not on my watch 😁

@schlunma
Copy link
Contributor

Perfect, thanks both!!

@schlunma schlunma merged commit 99df285 into main Jan 25, 2023
@schlunma schlunma deleted the fix_tests_with_core_dev_2 branch January 25, 2023 16:31
@valeriupredoi
Copy link
Contributor Author

Cheers @schlunma and @remi-kazeroni 🍺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Github Actions] Test in full dev mode (with dev esmvalcore) fails
3 participants