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

Include expected warning text in @test_logs #176

Merged
merged 1 commit into from
Jun 28, 2022

Conversation

tsj5
Copy link
Collaborator

@tsj5 tsj5 commented Jun 28, 2022

PULL REQUEST

Purpose and Content

Purpose

Change use of @test_logs to allow unit tests to pass when dependencies raise deprecation warnings. The writeup in issue #175 makes the case that the solution implemented here is the best way to deal with the case where the deprecation warning is raised in code we don't control.

Content

For all uses of @test_logs in unit tests,

  1. add a filter to match the text of the expected warning message,
  2. add match_mode=:any, to allow the test to pass when deprecation errors are raised.

Benefits and Risks

Benefits

Unit test suite now passes without needing to wait for the deprecations in Distributions.jl to be fixed in a new release;

Risks

If other, "serious" warnings are raised in addition to the warning being tested for, this implementation will give a false negative. As discussed in issue #175, as far as I know there's no way to optionally filter out deprecation warnings only.

Linked Issues

PR Checklist

  • This PR has a corresponding issue OR is linked to an SDI.
  • I have followed CliMA's codebase contribution and style guidelines OR N/A.
  • I have followed CliMA's documentation policy.
  • I have checked all issues and PRs and I certify that this PR does not duplicate an open PR.
  • I linted my code on my local machine prior to submission OR N/A.
  • Unit tests are included OR N/A.
  • Code used in an integration test OR N/A.
  • All tests ran successfully on my local machine OR N/A.
  • All classes, modules, and function contain docstrings OR N/A.
  • Documentation has been added/updated OR N/A.

@tsj5 tsj5 changed the title Include expected warning text in @test_logs [WIP] Include expected warning text in @test_logs Jun 28, 2022
@tsj5 tsj5 requested a review from ilopezgp June 28, 2022 05:21
Copy link
Contributor

@ilopezgp ilopezgp left a comment

Choose a reason for hiding this comment

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

LGTM

@tsj5 tsj5 changed the title [WIP] Include expected warning text in @test_logs Include expected warning text in @test_logs Jun 28, 2022
@tsj5
Copy link
Collaborator Author

tsj5 commented Jun 28, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 28, 2022

Build succeeded:

@bors bors bot merged commit 5ef0314 into CliMA:main Jun 28, 2022
@tsj5 tsj5 deleted the tsj/test-logs-filters branch June 28, 2022 19:30
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.

Make use of @test_logs less brittle
2 participants