GeezerVison

@Zac-HD the fact we had to add this seems to indicate a possibly-serious misuse of hypothesis, but in some way that @keewis and I struggled to properly understand from looking at the docs.

https://hypothesis.readthedocs.io/en/latest/settings.html#hypothesis.HealthCheck.differing_executors

see HypothesisWorks/hypothesis#3446 for the motivating cases; if you inherit an @given() test onto multiple child classes with different behavior you can get some pretty weird behaviors.

If you don't observe anything like that, it's probably okay albeit fragile.

In the very limited running we did today, we didn't observe anything unexpected after we disabled the health check.

But is there some other pattern we should be using here?

I don't have any concrete suggestions; inheritance for code-sharing is both useful in this kind of situation, and also prone to sharing slightly more state than we want it to. A design that doesn't use inheritance would be safer but I'm not sure it's worth the trouble.

Okay thanks. Sounds like perhaps we should disable this warning globally (if possible) and just report if it actually causes problems.

if you inherit an @given() test onto multiple child classes with different behavior

We are not actually ever going to be inheriting one given test onto multiple child classes, only onto one child class (per downstream package). So maybe that makes it okay?

...actually the one exception to that statement would be in Xarray itself, where we would inherit once to test wrapping numpy, once to test wrapping dask etc. But we could probably still set up our CI to ensure that only one child test class (suite of children really) gets run per CI job.

Should be fine, iirc it's only an issue if you're replaying test cases from the database and the underlying sequence of choices is different and you hit a particular unlucky situation.

actually the one exception to that statement would be in Xarray itself

not only that, unfortunately: if you want to check how dask and cupy work together, for example, cupy-xarray would have to create both the suite for cupy and the dask+cupy one.

The other option we'd have is to generate a single test class within a function:

def generate_tests(name, array_strategy, array_type, xp):
    @rename_class(f"Test{name.title().replace('_', '')}Array")
    class TestDuckArray:
        class TestCreation:
            array_strategy_fn = array_strategy
            ...

    return TestDuckArray

TestNumpyArray = generate_tests("numpy", create_numpy_array, np.ndarray, np)

which would avoid the reuse of a single given, but this quickly becomes tricky to read because of the deeply nested structure.

I've tried to work around this in #7 by delaying the application of given.

Whilst #7 is clever I think my conclusion from @Zac-HD 's comments above is that the extra complexity is unnecessary - it's fine to just suppress the warning and we only need to revisit this issue if we ever actually observe weird behaviour (i.e. YAGNI).

Consider using a Hypothesis-provided seed? I'd also be happy to accept a PR to generate Numpy prng instances 🙂

we should probably try using xps.arrays() instead (though I guess that only works for array API compliant duck arrays)

The other argument against is that sometimes you just want a faster PRNG for the elements; the distribution is a bit less likely to find bugs but setting elements individually is a lot slower (even though we do a sparse subset)