Losing coverage of Hypothesis-strategy-called code

[altendky]

This is a followup from discussions in #280.

As of Hypothesis 3.29.0 in September, coverage data is no longer collected for code called from strategies. It seems to have changed in the first commit or two after 3.28.3 (HypothesisWorks/hypothesis@6e7a478 HypothesisWorks/hypothesis@d75ac34). This was done intentonially to reduce the run time of tests.

This was identified after adjusting handling of unspecified metadata in attr.ib() (#278). The tests had no coverage reported for the case of metadata being specified despite that being the case many times during a test run (hundreds?). A little test was added to satisfy the coverage report.

attrs/tests/test_make.py

Lines 848 to 858 in b3861d1

	def test_metadata(self):
	"""
	If metadata that is not None is passed, it is used.
	This is necessary for coverage because the previous test is
	hypothesis-based.
	"""
	md = {}
	a = attr.ib(metadata=md)
	assert md is a.metadata

This does the job, so to speak, but it seems a bit odd to have hundreds of instances where metadata is being specified and only one reporting coverage. In no particular order, here are a few options.

• Talk with Hypothesis about changing this back (they were open to discussion)
• Adjust our strategies to generate parameters for attr.ib() calls instead of having them actually make the attr.ib() calls as they do now.
  • Here is a sample with a couple tests adjusted altendky@da4512b
• Setup isolated Hypothesis tests and separate 'regular' tests where only the 'regular' tests count towards coverage

There is some concern about using Hypothesis driven testing for coverage reports since Hypothesis can be inconsistent (I think that is what has been said). I am not sure why it's ok to trust Hypothesis will be consistent enough for functionality checks but not for coverage checks. With this issue being my first involvement with Hypothesis, perhaps I'm missing some of the implications and techniques of using it.

[wsanchez]

Does the coverage problem have any relation to this ticket?

As to whether hypothesis is OK for coverage testing, I assume that you mean that it’s possible that hypothesis will not generate test data that exercises certain code paths. If that’s the case, it’s possible that the unit tests aren’t specific enough, but you can also ensure that some data of the sort you need is always included by using the @example decorator, which is similar to how one might write a test without hypothesis but has the benefit of also having hypothesis try additional data.

[altendky]

@wsanchez, I don't think it's directly related. In this case I specifically observed that code called by a strategy (or drawing from a strategy?) did not trigger coverage but code called from inside a test did count towards coverage. The commit I linked (altendky/attrs@da4512b) had full coverage both before and after. The non-Hypothesis test test_metadata() was required to provide coverage before but once I refactored the Hypothesis tests to call attr.ib() inside of the test, rather than as part of the draw, they were sufficient to achieve coverage. So it seems that unlike the issue you linked there is no issue with coverage reporting of code inside the test.

If I remember correctly, it was @DRMacIver and @hynek that expressed some amount of uncertainty about using Hypothesis for coverage. My expectation was like yours, I think. Pick strategies that are guaranteed to achieve coverage. If at some point you lose coverage based on a test run triggered by an unrelated commit then you will get a surprising coverage error and will have to diagnose it and correct it. It seems that will just be the way it goes unless you fully avoid recording any Hypothesis-test-called coverage info. Unless maybe you can run @example inputs independently of regular Hypothesis draws? Maybe then one test site can be maintained but coverage only checked in statically defined inputs.

[Zac-HD]

We ended up removing the coverage features in Hypothesis 3.71 (HypothesisWorks/hypothesis#1564), because they were fairly brittle and did not interact well with the rest of the ecosystem - and with only a hundred or so runs, targeted exploration is out-performed by faster execution.

[wsanchez]

Is this still an issue given the prior comment?

[altendky]

I'll have to read some more later. I'm not clear what was removed. I can't say I know for sure but my impression was that hypothesis added a thing that explicitly ended up blocking coverage from working soon after 3.28.3... I guess a closure of this ticket (if/when it happens) should maybe include an explicit statement that attrs does or does not expect attrs code called by drawing from hypothesis strategies is expected to be included in coverage reports. Hopefully that's correct terminology. Whichever is chosen we should make sure hypothesis is doing this. Maybe even add an explicit test to confirm it somehow.

I don't know that Bug is a proper label for this. I see this ticket as more of a 'policy needs to be decided' thing and then maybe other tickets/actions follow from such a decision.

[wsanchez]

OK, swapped Bug for Thinking.

[Zac-HD]

Hypothesis 3.29 added some coverage-based searching, which turned out to be a net loss partly on the grounds that it interfered with test suite coverage measurement.

It was therefore removed in 3.71 more than a year ago, and will not return.

So unless you're pinning to a very old version (please don't!), this issue can just be closed.

[altendky]

Ok, I read through https://hypothesis.readthedocs.io/en/latest/changes.html#v3-29-0 now. So I guess the loss of coverage reporting for code exercised by drawing from a strategy was a side effect of that effort?

If I remember correctly, it was @DRMacIver and @hynek that expressed some amount of uncertainty about using Hypothesis for coverage.

Because of this it seems maybe useful to have an explicit decision of what is wanted. I (and I think @wsanchez) both are fine with expecting hypothesis-triggered code (outside the body of a test function) being considered as part of coverage results. We explicitly design and pick the strategies to achieve functionality coverage, I'm not sure why code coverage has a higher bar. But, @hynek seemed to have a different opinion and it seems that what we can expect to get from Hypothesis now is not inline with @hynek's preference (per my recollection of and notes about a discussion from two whatever years ago so... yeah).

The coverage loss was addressed by adding a test. I suppose it could be removed now but both those points are sort of separate from this issue. This issue is to decide what attrs wants, not to deal with a Hypothesis bug/change/whatever. Of course, maybe those actually responsible for this repo will decide discussion/clarity isn't needed on this point and move on. I have no particular need either way on this at the moment.

[hynek]

Hm I think we'll just roll with the punches. I'm trying to test nothing by accident but if the coverage ever drops under 100% we'll cope with it.

[hynek]

Fun fact: I just had a coverage fail on our ReadOnlyDict which has zero tests as I've noticed, so there's my next todo item. 🤪