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

get rid of test_approx_eq_* #4615

Closed
StefanKarpinski opened this issue Oct 23, 2013 · 18 comments
Closed

get rid of test_approx_eq_* #4615

StefanKarpinski opened this issue Oct 23, 2013 · 18 comments
Labels
testsystem The unit testing framework and Test stdlib
Milestone

Comments

@StefanKarpinski
Copy link
Sponsor Member

These are not really fundamental testing primitives and should be replaced with @test isapprox(...) or something similar.

@JeffBezanson
Copy link
Sponsor Member

The point of this is it knows more about what is being tested, so it can print the compared values and their difference compared to the requested tolerance, instead of just a boolean result.

@StefanKarpinski
Copy link
Sponsor Member Author

Well, we need a better general way of printing more information when tests fail. As it is, these fail incorrectly: they throw an error instead of failing.

@JeffBezanson
Copy link
Sponsor Member

Ah, well failing incorrectly is certainly a problem.

@StefanKarpinski
Copy link
Sponsor Member Author

Can't tell if serious...

@JeffBezanson
Copy link
Sponsor Member

I was actually serious, but you have a point.

@andreasnoack
Copy link
Member

I have aggressively changed @test to @test_approx_eq in the test code I have met because of the extra output. In the linalg test code the line numbers relate to the end of the loop that goes through all the BlasTypes. Therefore it can be difficult to know which test is failing unless you can see some output.

@StefanKarpinski
Copy link
Sponsor Member Author

Well, I'm working on improving the @test macro to be more informative when tests fail, which should eliminate the need for these things.

@IainNZ
Copy link
Member

IainNZ commented Oct 24, 2013

Having @test display more would be great! We do currently use test_approx_eq_eps in @JuliaOpt code but any reasonable replacement will be fine.

@simonbyrne
Copy link
Contributor

I support getting rid of @test_approx_eq_*, as they're kind of confusing: @test_approx_eq is relative-error based (but at a fixed threshold), and @test_approx_eq_eps is absolute-error based. Forcing use of isapprox would make it easier to set more reasonable thresholds in tests.

I also agree that @test output needs to be improved, ideally outputting the values of all inputs into the failed test.

@stevengj
Copy link
Member

Given #12472, you can now do @test foo ≈ bar and you will get the nice test output, as long as you are happy with the default tolerance.

It would be fairly easy to update the @test macro to look for @test isapprox(...) too, for the cases where a non-default tolerance is used.

@kshyatt kshyatt added testsystem The unit testing framework and Test stdlib and removed test This change adds or pertains to unit tests labels Sep 15, 2016
@kshyatt
Copy link
Contributor

kshyatt commented Sep 15, 2016

@StefanKarpinski didn't your great Regex-fest (mostly) do this?

@tkelman
Copy link
Contributor

tkelman commented Sep 15, 2016

They're not used much in base any more, but they should be deprecated to really close this if we want to finish getting rid of them all the way.

@ViralBShah ViralBShah added this to the 0.6.0 milestone Sep 15, 2016
@timholy
Copy link
Sponsor Member

timholy commented Sep 15, 2016

Related: #15477

@IainNZ
Copy link
Member

IainNZ commented Sep 15, 2016

I really did want to kill these during the at-testset change, but failed to find a solution I liked at the time.

@StefanKarpinski
Copy link
Sponsor Member Author

Bump?

@stevengj
Copy link
Member

A good solution would be to recognize @test isapprox(...) and give extra output for it (e.g. the two things being compared, and the relative error encountered vs. the error tolerance requested).

juliangehring added a commit to juliangehring/MultipleTesting.jl that referenced this issue Dec 26, 2016
Addresses
- [JuliaLang/julia#15477]: Gives us better
control for defining tolerance levels
- [JuliaLang/julia#4615]: Fixes test cases not
being represented in the testcase summary
@JeffBezanson JeffBezanson modified the milestones: 1.0, 0.6.0 Jan 5, 2017
StefanKarpinski added a commit that referenced this issue Jan 5, 2017
`@test_approx_eq_eps`, you're next!
StefanKarpinski added a commit that referenced this issue Jan 6, 2017
Instead, define Test.test_approx_eq_modphase in the two files where
it is needed. This is a *very* specific function and certainly not
one that needs to live in Julia's standard library code.

Closes #4615.
@StefanKarpinski
Copy link
Sponsor Member Author

I have a PR for most of this (#19901), the remaining special printing of failures for @test a ≈ b is split out into #19898, which can be handled whenever since it's just a display improvement.

ViralBShah pushed a commit that referenced this issue Feb 11, 2017
@KristofferC
Copy link
Sponsor Member

This is done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testsystem The unit testing framework and Test stdlib
Projects
None yet
Development

No branches or pull requests