Use SafeTestsets?

[devmotion]

This PR is a follow-up to the discussion in #63. It shows how a test setup with SafeTestsets could look like.

Additionally, I fixed and enabled the tests of the Mahalanobis kernel.

[theogf]

I guess the begin end cannot be removed?

[devmotion]

No, since it ends up being just a @testset that is wrapped inside its own module and @testset needs a begin/end block or a for loop:

  LoadError: Expected begin/end block or for loop as argument to @testset

[willtebbutt]

Is there a particular reason for this, rather than putting the @safetestset inside the test file it's targetting?

[devmotion]

It just felt more natural to me. I thought, the main motivation for using @safetestset is not to mix imports and definitions of different test files, and hence it is only/mostly relevant when including different test files such as in runtests.jl.

[willtebbutt]

Ah I see.

It just felt more natural to me.

Personally I think I would prefer to have a new @safetestset in each test file so that when you run it by itself, you get a new testset. So the test files would be of the form

@safetestset "foo" begin
    using KernelFunctions
    using Test
    using Random

    @testset "some tests" begin

    end

    @testset "some more tests" begin

    end
end

This would tidy up the runtests.jl file a bit, and make the results of running a particular test suite nicely encapsulated if I run include("test/mytests.jl"). If you don't include stuff inside a @safetestset in each file you'll add state to your global state if running from the REPL.

edit: actually, I think this last global state bit is probably the most important bit, and a good technical reason.

[theogf]

Probably it's a bad practice but I actually like to have a global state when some debugging is needed!

[willtebbutt]

Sure, but it's absolutely not something that you want when you're running your test sets. I can get on board with producing some global state to figure out what's going on with a particular thing, but then I want to be able to re-run my tests and know that I'm not going to get different results if I restart my repl session.

edit: if you want to produce some global state from your test sets, you can just copy + paste some code from the test set to the repl, but I definitely think you want to be able to run include("test/some_tests.jl") and know that what you've been doing at the repl isn't going to effect the results.

[theogf]

Anyway I agree your point is stronger than my lazy practices 😄

[willtebbutt]

I am really not a big fan of the REPL copy paste

😆 that's totally fair, can definitely get on board with that.

[devmotion]

I agree, it seems reasonable to expect that includeing the test file should not affect the global state. However, it seems the main advantage of @safetestset over just having separate modules in every test file would be that the names of the modules are generated with gensym. I'm not sure if that justifies the use of @safetestset, in particular since one has to import SafeTestsets before including such a test file whereas including a test file that defines its own module would just work. So it might actually be more reasonable to not use SafeTestsets?

[willtebbutt]

I can't say that I'm overly concerned at the idea of having to load SafeTestsets to be able to run the files -- that wouldn't be a deal breaker from a workflow perspective I don't think. I'm easy on this one -- if people would prefer to use SafeTestsets and are happy to load it before including particular files, that's fine by me. I'm also completely happy with our current test set up.

We could always revisit this is in the future if we feel it's worthwhile.

[devmotion]

I'll close this PR for now since I'm also happy with the current setup. And as you say, we can always revisit this in the future.