Test the unhappy path #60

oxinabox · 2020-10-06T17:18:37Z

This PR is about testing that we can actually detect when things are wrong.

The code for metatest_get_failures is a pretty nasty hack an on the internals.
I think its clear enough what it is doing.

Two possible alternative implementations:

Alt 1: use a seperate process.

Julia itself does this to test @test.
Writing a test to a file and then kicking it off with run(`julia -e file.jl`)
But three problems are

Slow starting an external process is slow
Very course grained: all that is easy to test is that it caused julia to to exit with a nonzero status
Pretty ugly code, need to redirect stdout still and need to write a file

Alt 2: custom Testset type

We could declare a custom testset type which overloads record and finish so that they don't propate errors upwards.
This might be fiddly, but I have done something like it before in https://github.com/JuliaTesting/ReferenceTests.jl.
so could base it off that.
It should be simplier than that.
It could be pretty clean maybe. Not 100% sure.

oxinabox · 2020-10-06T17:18:58Z

test/meta_testing_tools.jl

+Invoking it via `metatest_get_failures(f)` will prevent those `@test` being added to the
+current testset, and will return a collection of all nonpassing test results.
+"""
+function metatest_get_failures(f)


Should this just be called

Suggested change

function metatest_get_failures(f)

function get_failures(f)

?

yeah, extract_failures or fail_results (since they are Test.Fail <: Test.Result) or just failures?

They are any non-pass.
So they could be Errors, or some other cause, I don't recall what @test_broken returns.
Arguably we should call it nonpasses

yeah nonpasses is better (to avoid the failures = Fail confusion i had)

test_broken also treats all non-passes the same

julia> @test_broken 1 == 2 Test Broken Expression: 1 == 2 julia> @test_broken error() Test Broken Expression: error()

test/testers.jl

nickrobinson251

(reviewed with @bencottier)

one design question, and a couple naming comments, but otherwise i think the broadly implementation looks good -- i've asked Ben to follow-up with any comments on the implementation later this afternoon

overall Alt 1 and (probably) Alt 2 both seem less nice to us

nickrobinson251 · 2020-10-07T10:56:48Z

test/testers.jl

+        @testset "unhappy path" begin
+            alt_double(x) = 2x
+            @scalar_rule(alt_double(x), 3)  # this is wrong, on purpose
+            failures = metatest_get_failures(()->test_scalar(alt_double, 2.1))


we want to test that test_scalar returns a test failure (rather than a pass or an error), when the @scalar_rule is wrong -- is that right?

i wonder if the testset name can be more precise on that, than "unhappy path"

we want to make sure it doesn't succeed.

I can't think of a better name.
Unhappy Path is a term of literature
https://cucumber.io/blog/test-automation/happy-unhappy-paths-why-you-need-to-test-both/

we want to make sure it doesn't succeed.

i am confused by this -- why don't we only care about failures?
(also current naming is confusing to me if it includes more than Test.Fails)
https://github.com/JuliaDiff/ChainRulesTestUtils.jl/pull/60/files#r501079347

this is also why i prefer to be more precise than "unhappy path" ...unless we explicitly document that "unhappy path" means "anything that doesn't result in only Test.Pass"

nickrobinson251 · 2020-10-07T11:34:54Z

test/meta_testing_tools.jl

+Invoking it via `metatest_get_failures(f)` will prevent those `@test` being added to the
+current testset, and will return a collection of all nonpassing test results.
+"""
+function metatest_get_failures(f)


yeah, extract_failures or fail_results (since they are Test.Fail <: Test.Result) or just failures?

test/meta_testing_tools.jl

test/testers.jl

nickrobinson251 · 2020-10-07T11:49:36Z

test/testers.jl

+                return 2.5 * x, identity_pullback
+            end
+
+            @test !isempty(metatest_get_failures(()->frule_test(my_identity1, (2.2, 3.3))))


I do wonder if there's an alternative design (or just another layer of abstraction?)
so we end up with a call more like

@test_failure frule_test(my_identity1, (2.2, 3.3))

or

@test fails(() -> frule_test(my_identity1, (2.2, 3.3))

?

Yeah i think the later makes sense.

out of interest, why not the macro (that just wraps the call in an anonymous function and passes it to fails)?

the macro looks like @test_throws and @test_broken
but it does not at all work like those do.

@test_throws catchs the exception.
@test_broken x is very similar to @test !x + some extra stuff

I would worry that people might think @test_failure x is the same as same as @test !x

people might think @test_failure x is the same as same as @test !x

yeah, great point.

test/testers.jl

Co-authored-by: Nick Robinson <npr251@gmail.com>

oxinabox · 2020-10-07T12:33:09Z

@nickrobinson251 you didn't comment on if either of the Alternative Implementations mentioned in the top post might be better.

nickrobinson251 · 2020-10-07T12:36:10Z

@nickrobinson251 you didn't comment on if either of the Alternative Implementations mentioned in the top post might be better.

We did (above). We think Alt 1 is clearly worse. Alt 2 we didn't think too hard about but I couldn't see what we'd gain over the current implementation

oxinabox · 2020-10-07T13:43:50Z

Ah i missed it

overall Alt 1 and (probably) Alt 2 both seem less nice to us

test/meta_testing_tools.jl

bencottier · 2020-10-07T17:26:19Z

test/meta_testing_tools.jl

+_extract_failures(ts::Test.DefaultTestSet) = _extract_failures(ts.results)
+function _extract_failures(xs::Vector)
+    if isempty(xs)
+        return Test.Result[]


If f actually doesn't call @test, we end up here -- is this the best thing to return in that case?

I think so?

nickrobinson251

💚

test/meta_testing_tools.jl

Co-authored-by: Nick Robinson <npr251@gmail.com>

test/testers.jl

oxinabox · 2020-10-09T13:18:29Z

Not going to tag a release off this as it is not a functional change

oxinabox added 4 commits October 6, 2020 17:37

Introduce meta testing tools

753d9f6

mute get_failures

6c66052

Test unhappy path

29659af

add comment to meta testing tools

59babd1

oxinabox commented Oct 6, 2020

View reviewed changes

test/testers.jl Outdated Show resolved Hide resolved

Redirect to tempfile as no redirect_stdout(devnul) until julia 1.5

3ce5441

nickrobinson251 self-requested a review October 7, 2020 10:28

nickrobinson251 reviewed Oct 7, 2020

View reviewed changes

oxinabox mentioned this pull request Oct 7, 2020

Move meta testing utilities into a new MetaTesting.jl package #62

Closed

fix spacing

56e1aa4

Co-authored-by: Nick Robinson <npr251@gmail.com>

nickrobinson251 reviewed Oct 7, 2020

View reviewed changes

test/meta_testing_tools.jl Outdated Show resolved Hide resolved

bencottier reviewed Oct 7, 2020

View reviewed changes

introduce and use fails to check for failures

2217648

nickrobinson251 approved these changes Oct 7, 2020

View reviewed changes

test/meta_testing_tools.jl Outdated Show resolved Hide resolved

test/meta_testing_tools.jl Outdated Show resolved Hide resolved

test/meta_testing_tools.jl Outdated Show resolved Hide resolved

test/meta_testing_tools.jl Outdated Show resolved Hide resolved

fix missed changes to nonpasses

1954660

Co-authored-by: Nick Robinson <npr251@gmail.com>

oxinabox commented Oct 8, 2020

View reviewed changes

test/testers.jl Outdated Show resolved Hide resolved

Remove extra bracket

6f1fe74

oxinabox merged commit 1bfe367 into master Oct 9, 2020

oxinabox deleted the ox/metatest branch October 9, 2020 13:18

nickrobinson251 mentioned this pull request May 19, 2023

Renaming test functions? JuliaTesting/MetaTesting.jl#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test the unhappy path #60

Test the unhappy path #60

oxinabox commented Oct 6, 2020

oxinabox Oct 6, 2020

nickrobinson251 Oct 7, 2020

oxinabox Oct 7, 2020

nickrobinson251 Oct 7, 2020

nickrobinson251 left a comment •

edited

Loading

nickrobinson251 Oct 7, 2020

nickrobinson251 Oct 7, 2020

oxinabox Oct 7, 2020

nickrobinson251 Oct 7, 2020

nickrobinson251 Oct 7, 2020

nickrobinson251 Oct 7, 2020

nickrobinson251 Oct 7, 2020 •

edited

Loading

oxinabox Oct 7, 2020

nickrobinson251 Oct 7, 2020

oxinabox Oct 7, 2020

nickrobinson251 Oct 7, 2020 •

edited

Loading

oxinabox commented Oct 7, 2020

nickrobinson251 commented Oct 7, 2020 •

edited

Loading

oxinabox commented Oct 7, 2020

bencottier Oct 7, 2020

oxinabox Oct 7, 2020

nickrobinson251 left a comment

oxinabox commented Oct 9, 2020

Test the unhappy path #60

Test the unhappy path #60

Conversation

oxinabox commented Oct 6, 2020

Alt 1: use a seperate process.

Alt 2: custom Testset type

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickrobinson251 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickrobinson251 Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickrobinson251 Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

oxinabox commented Oct 7, 2020

nickrobinson251 commented Oct 7, 2020 • edited Loading

oxinabox commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickrobinson251 left a comment

Choose a reason for hiding this comment

oxinabox commented Oct 9, 2020

nickrobinson251 left a comment •

edited

Loading

nickrobinson251 Oct 7, 2020 •

edited

Loading

nickrobinson251 Oct 7, 2020 •

edited

Loading

nickrobinson251 commented Oct 7, 2020 •

edited

Loading