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 UndefVarError in Julia v1.0-1.3 #73

Merged
merged 2 commits into from
Mar 25, 2022
Merged

Conversation

giordano
Copy link
Contributor

Ref: https://github.com/JuliaTesting/Aqua.jl/pull/71/files#r834842406. If you have better ideas about how to more proper fix this or how to actually test this code path which is clearly untested, please take over this PR or open a new one, but at least this doesn't break the package in older versions of Julia. Not very useful to have a QUality Assurance package that breaks your tests.

CC: @Roger-luo

@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #73 (18e3ac5) into master (b1f583a) will decrease coverage by 0.11%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
- Coverage   73.66%   73.54%   -0.12%     
==========================================
  Files          10       10              
  Lines         619      620       +1     
==========================================
  Hits          456      456              
- Misses        163      164       +1     
Flag Coverage Δ
unittests 73.54% <66.66%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/ambiguities.jl 82.85% <66.66%> (-0.80%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@tkf tkf merged commit a4773dc into JuliaTesting:master Mar 25, 2022
@tkf
Copy link
Member

tkf commented Mar 25, 2022

Thanks! It'd be nice to actually hit this case in the test but let's register this version first. JuliaRegistries/General#57279

If you have some simple-ish examples for exercising #71, it'd be great if you can add some in https://github.com/JuliaTesting/Aqua.jl/blob/master/test/pkgs/PkgWithAmbiguities.jl (cc @Roger-luo)

@Roger-luo
Copy link
Contributor

Yeah I'm not sure how to test the checks tho, but I can have a look at that morespecific function

@tkf
Copy link
Member

tkf commented Mar 25, 2022

That'd be great. But I'm also OK with just doing a "smoke test" to hit the code using morespecific with some MWE functions. It won't be a comprehensive test but it's still useful since morespecific is an internal function. (And many of Aqua's tests are smoke tests anyway.)

@giordano giordano deleted the patch-1 branch March 25, 2022 23:09
@giordano
Copy link
Contributor Author

For what is worth, I found this error in JuliaPhysics/Measurements.jl#82, but I have little clue of where the ambiguity comes from: tests are passing on master, and that PR is only changing the signature of an internal macro, in addition to loading a new package (FiniteDifferences instead of Calculus). Can the ambiguity come from the new package?

Thanks for the quick merge! And sorry for the salty comment above, but a failure from a testing package triggered me yesterday night 🙂

@tkf
Copy link
Member

tkf commented Mar 26, 2022

Oh, no, that's OK :) I totally missed that my test didn't hit the code.

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