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 test warning #292

Merged
merged 8 commits into from
Mar 27, 2024
Merged

Fix test warning #292

merged 8 commits into from
Mar 27, 2024

Conversation

jasonjunweilyu
Copy link
Collaborator

@jasonjunweilyu jasonjunweilyu commented Mar 14, 2024

This PR is to fix test warnings, either by changing them to report errors when warnings are not issued if they are designed in fab with intentions through pytest.warns() or by filtering them out if they are related to deprecation warnings in dependent packages with @pytest.mark.filterwarnings(). The motivation of this PR is explained in Issue hiker/fab#5. The fix for test errors is in another PR.

This PR has passed the CI pipeline. It was tested to have no warnings with Python 3.12. However, there are still some Python-version-specific warnings left if you look at the CI pipeline results. The related PR on hiker/fab is here: hiker/fab#10

Depends on

Copy link
Collaborator

@MatthewHambley MatthewHambley left a comment

Choose a reason for hiding this comment

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

A lot of the log chatter is probably left over development nonsense however this change does neatly document what exists.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I seem to recognise this change from a different PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This also looks familiar although it has new stuff too. Has there been a branching issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another duplicate change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seen in a different PR.

@hiker
Copy link
Collaborator

hiker commented Mar 14, 2024

Hi Matthew,
thanks a lot for the very quick review. Note that Jason is on leave, and I've asked him to put this PR in before he goes, but this PR needs to be applied after #291 (since without this patch we can't get CI to give us all green on our system, so it is dependent on that). Unfortunately, we can't (currently) set a label to indicate which PR is ready and which isn't :( I will try to fix #291 for Jason, and update this PR then.

…ith changes in fix_test_errors, with conflicts solved
@jasonjunweilyu
Copy link
Collaborator Author

Hi @MatthewHambley , I have merged metomi:master into hiker:fix_test_warning after the pull request of merging hiker:fix_test_error into metomi:master was approved. Now the duplicated change issues should have all gone, and this pull request is ready for review. Thanks.

Copy link
Collaborator

@MatthewHambley MatthewHambley left a comment

Choose a reason for hiding this comment

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

I think this is ready for trunk.

@MatthewHambley MatthewHambley merged commit e51cbfb into MetOffice:master Mar 27, 2024
4 checks passed
@jasonjunweilyu jasonjunweilyu deleted the fix_test_warning branch March 28, 2024 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants