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

Make use of @test_logs less brittle #175

Closed
tsj5 opened this issue Jun 28, 2022 · 0 comments · Fixed by #176
Closed

Make use of @test_logs less brittle #175

tsj5 opened this issue Jun 28, 2022 · 0 comments · Fixed by #176
Assignees

Comments

@tsj5
Copy link
Collaborator

tsj5 commented Jun 28, 2022

This issue is raised to describe problems encountered here with getting the test suite to pass for an unrelated PR (#161). I've been able to reproduce it locally on main as well (since it originates in a dependency).

Description

The linked test suite failure arises from a unit test of the form @test_logs (:warn,): when run under GH actions, the test gets two warnings, instead of the single warning it expected, and fails.

The second warning isn't related to EKP: it's a deprecation warning raised by MvNormal:

┌ Warning: `dim(a::AbstractMatrix)` is deprecated, use `LinearAlgebra.checksquare(a)` instead.
│   caller = MvNormal at mvnormal.jl:186 [inlined]
└ @ Core ~/.julia/packages/Distributions/9Albf/src/multivariate/mvnormal.jl:186

I can reproduce this locally: it arises because PDMats.jl deprecated dim in its latest release, two days ago as of this writing, and Distributions.jl hasn't been updated.

Proposed fix

I propose to fix this by modifying EKP's usage of @test_logs to 1) test for the text of the specific, expected warning message, and 2) to ignore any other warnings via match_mode=:any.

  • It's not feasible to pin the version of PDMats in EKP's Project.toml; it's a dependency of a dependency (LinearAlgebra) and we'd have to remember to unpin and update it when Distributions.jl fixes its deprecation warnings.
  • At the same time, we should keep --depwarn=yes in EKP's test suite, to warn about deprecations in code we control.
  • @test_logs provides some functionality for filtering log messages, but the filters are applied in one-to-one order with the log messages raised; there doesn't appear to be a way to filter out logs that may or may not be present, except through the use of match_mode=:any.
@tsj5 tsj5 changed the title Make use of \@test_logs less brittle Make use of @test_logs less brittle Jun 28, 2022
@tsj5 tsj5 self-assigned this Jun 28, 2022
@bors bors bot closed this as completed in 5ef0314 Jun 28, 2022
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 a pull request may close this issue.

1 participant