Make _CountingAttr empty metadata unique

[altendky]

Issue #278

[codecov]

Codecov Report

Merging #280 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #280   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         729    731    +2     
  Branches      151    152    +1     
=====================================
+ Hits          729    731    +2

Impacted Files	Coverage Δ
src/attr/_make.py	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ed8c4a...6be39c2. Read the comment docs.

[altendky]

No coverage for non-default metadata? Hmm. Locally metadata is mostly None at the point of the partially covered if metadata is None: but many times it is not None so I'm not sure I understand this correctly. Guidance would be appreciated.

Note that this is my first Hypothesis test and while my change to the bare_attrs makes sense to me it does seem fishy that I would need to modify that. If there's another way to get independent instances of _CounterAttr or if we want to leave existing as is and just do what I did only for the new test, let me know.

Also, I am not sure if this belongs in any docs or changelog. It seems a bit behind the scenes but not completely.

[hynek]

@Tinche this is yours

[Tinche]

Sure, I'll take this when I'm feeling a little better.

[hynek]

Sure no rush, this is not a blocker.

[altendky]

@Tinche No urgency for me. I've got my simple workaround in place. I hope you get better soon.

[Tinche]

Rebase please and I will review.

[altendky]

Updated, but all the caveats from #280 (comment) still apply. I'd love to understand the coverage issue.

[Tinche]

I'm not sure what's going on with the coverage data.

[altendky]

@Tinche switched to bare_attrs = st.builds(attr.ib, default=st.none())

[Tinche]

I'm OK with this PR as it stand except I'd like to get to the bottom of the coverage issue. @hynek any ideas?

[altendky]

If you don't know I'll see what I can find. Probably not for a couple days though. Thanks for keeping up on this.

[hynek]

So this is interesting.

If I do the famous XXX test:

    if metadata is None:
        XXX
        metadata = {}

it fails already in hypothesis. If I invent an else, it fails in our test suite:

    if metadata is None:
        metadata = {}
    else:
        XXX

Therefore, if I add this test:

def test_meta():
    y = attr.ib(metadata={})

we get 100% test coverage.

IOW we don’t appear to have a test that sets metadata to something which seems like a glaring omission.

[Tinche]

Hypothesis will generate attributes with arbitrary metadata in some tests. We probably never assert the metadata is the thing we set it to, though.

[altendky]

One of the first things I did was to print metadata and run the tests. It mostly printed None but many times printed a thing. I can't swear I printed the repr(), but I normally do. I'm away from my computer for a day or I'd try again. I also recall seeing 100% coverage reported locally.

@hynek, did you get less than 100% coverage when running locally?

[hynek]

Yes I had less than 100% until I added the test I mentioned which lifted it to 100%.

[Tinche]

Ok, just stick the test somewhere so we have 100% coverage again and I'm good with this. Also I think we need a changelog entry for this, even though this isn't a public thing we're changing? Can't hurt to add one.

[altendky]

I added some prints and confirmed that both sides of the if are being hit. If we look down at a run without disabling capture we see that under py36 runtests: commands[0] | coverage run --parallel -m pytest we do in fact only ever see metadata is None getting hit.

It sort of looks like coverage is not being measured during the full test run but only on that commands[0] line. Is there anything related to this that was intentional? I still have to learn a bit about coverage/tox/pytest to understand how these systems work.

I hope you don't mind me digging into this a bit more. The way I read all this is that the tests would cover metadata is None both ways but something about how we are running it, or just a coverage bug, is keeping it from being measured.

[altendky]

I added both a 'real' test that sure seems like it should achieve coverage (beyond all the other indications we should have had full coverage already) and also a 'force coverage' test that actually manages to achieve full reported coverage.

I have taken several passes at this without managing to identify the actual issue. Perhaps it relates in some way to HypothesisWorks/hypothesis#997? Though I tried the various mentioned workarounds there including HypothesisWorks/hypothesis#1031 without any effect. The only relationship I see is that hypothesis tests don't seem to consistently have coverage reported appropriately. Are you concerned enough to open a new ticket about this coverage funny business?

This gist shows the coverage loss when the coverage forcing test is removed.

@Tinche, was there anything else you had wanted changed? I added the news fragment and the coverage report doesn't complain anymore.

[Tinche]

@hynek please take a last look and do the honors. :)

[hynek]

Thanks and welcome to software hell. ;)

[altendky]

@hynek, yeah, so it goes. I made a sample showing that code called from strategies does not get coverage recorded. From a discussion in #hypothesis, apparently this is expected behavior. It changed at release 3.29.0 in September. Actually, the first commit or two after 3.28.3 (HypothesisWorks/hypothesis@6e7a478 HypothesisWorks/hypothesis@d75ac34). The recommendation was to not rely on strategies for coverage. One approach would be to use the strategy to provide the attrib() parameters and then do the actual attrib() call inside the test function. Maybe via a decorator instead?

In case you can't tell, I just hate sticking stuff in to cover up systems that aren't working as expected (well, at least as I expected :]). Should I open a ticket to track this? Or just drop it...

[altendky]

Apparently it may just be a performance thing and could be open to discussion. If you don't want to just drop it. Using @example was also brought up but the guess was that it may not be appropriate for the attrs case.

[hynek]

I actually welcome that feature! I was always a bit…nervous about relying on hypothesis for coverage because it’s kind of a matter of luck that it hits them all.

I’ll just rewrite the test myself, don’t worry about it.

[altendky]

This only seems to partially address any concerns about inconsistent coverage from using hypothesis. The test body that runs would be just as inconsistent about coverage as the construction in the strategies, I would think.

As opposed to a little change like that, I was thinking more that a large portion of the test suite may be based on the expectation that strategies provided coverage and might need to be reconsidered (even though we do have 100% coverage presently). I figured it was the developers responsibility to pick strategies such that coverage would be achieved. In this case being sure to explicitly use both simple_attrs_with_metadata and simple_attrs_without_metadata.

I went ahead and made a small example adjustment to try out the idea of having strategies only generate parameters and the test itself generate the attr.ib().

altendky@da4512b

If there is desire to simply not use hypothesis for coverage then it seems like there should perhaps be complete isolation with no coverage for hypothesis tests and separate tests which are the sole contributors to coverage. It seems that otherwise the uncertainty would remain.

[hynek]

scratches head

That’s some heavy shit to do just on the side man. :)

Generally speaking, I would like to streamline our hypothesis use (the test suite could be really faster) so I’m open to improvements (although they’ll have to pass @Tinche’s discerning eye :)).

So I would suggest to open a new issue and sum up your ideas & findings from here, thanks.