Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix test coverage #5579

Merged
merged 3 commits into from Jul 8, 2017
Merged

fix test coverage #5579

merged 3 commits into from Jul 8, 2017

Conversation

MartinNowak
Copy link
Member

  • use single tests as workaround for Issue 16397
  • fix single tests (broken sed command)

- use single tests as workaround for Issue 16397
- fix single tests (broken sed command)
@dlang-bot
Copy link
Contributor

dlang-bot commented Jul 8, 2017

Thanks for your pull request, @MartinNowak!

Bugzilla references

Auto-close Bugzilla Description
16397 missing coverage from template instances when using separate compilation

- was only privately imported anyhow
- merging is done with file locking by druntime
@MartinNowak MartinNowak requested a review from wilzbach July 8, 2017 18:40
Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MartinNowak
Copy link
Member Author

Only running the single tests (against a libphobos2.a w/o -cov) means we're no longer gathering coverage that happens from executing unittests of other modules, e.g. std.net.curl might use std.conv.
That seems actually like a saner model to work with, but might be problematic for unittests in external files (do we have that in phobos). That also explains why coverage for some modules decreased a bit.

@wilzbach
Copy link
Member

wilzbach commented Jul 8, 2017

Btw in these modules it decreases the coverage:

image

and in these it increases the coverage:

image

(not a complete screenshot)

I will have a look if coverSetMerge can help here to get the best from both worlds.

@CyberShadow
Copy link
Member

I will have a look if coverSetMerge can help here to get the best from both worlds.

That doesn't sound like the right approach.

If the change increases coverage in some places but decreases it in others, isn't it essentially replacing one broken solution with another broken solution?

@wilzbach
Copy link
Member

wilzbach commented Jul 8, 2017

That doesn't sound like the right approach.

Better ideas are welcome!

If the change increases coverage in some places but decreases it in others, isn't it essentially replacing one broken solution with another broken solution?

Yes.

@CyberShadow
Copy link
Member

@rainers Perhaps you could help us out with this one?

@jmdavis
Copy link
Member

jmdavis commented Jul 8, 2017

How on earth would only running the tests from a module when measuring its code coverage increase its code coverage? Does it have something to do with having fewer template instantiations? Regardless, it makes seem like there's a problem with the coverage analyzer.

@rainers
Copy link
Member

rainers commented Jul 9, 2017

@rainers Perhaps you could help us out with this one?

I'm not sure I can. I think Martin's assessment in https://issues.dlang.org/show_bug.cgi?id=16397#c1 sounds correct. I don't have a solution to that.

Always generating coverage instrumentation of templated code will only work if you cover all linked libraries, though. For example, coverage from druntime modules might leak into the report, just as well as that it might provide template instances without coverage instrumention.

@MartinNowak MartinNowak deleted the fix_coverage branch July 9, 2017 13:08
@MartinNowak
Copy link
Member Author

@wilzbach, @CyberShadow, @jmdavis, @rainers
Seems like noone read #5579 (comment), explains the decrease in some places. We no longer account for coverage as a collateral of running other module's tests. I think that's a sane behaviour but might run into issues for externalized tests such as std.regex.internal.tests.

@jmdavis
Copy link
Member

jmdavis commented Jul 10, 2017

Seems like noone read #5579 (comment), explains the decrease in some places.

The decrease makes sense to me (e.g. the tests for SysTime are primarily in std.datetime.timezone because of how tied they are to the time zones, and they were all in one module originally, so a decrease in std.datetime.systime's coverage wouldn't surprise me). It's the increase that doesn't make sense to me.

@MartinNowak
Copy link
Member Author

It's the increase that doesn't make sense to me.

Read the explanation in the bug report https://issues.dlang.org/show_bug.cgi?id=16397#c1, the linker picked non-instrumented template instantiations in various places.

@jmdavis
Copy link
Member

jmdavis commented Jul 10, 2017

Read the explanation in the bug report https://issues.dlang.org/show_bug.cgi?id=16397#c1, the linker picked non-instrumented template instantiations in various places.

Ah, okay. That makes sense. LOL. Some of this stuff is just way more complicated than it seems like it should be.

@CyberShadow
Copy link
Member

CyberShadow commented Jul 10, 2017

Seems like noone read #5579 (comment),

Sorry Martin, you're right, I only skimmed over the explanations. At the moment I was a bit overwhelmed and didn't grok it.

I guess then we have two three(?) ways going forward:

  • Change DMD's coverage behaviour for external modules as described in issue 16397
  • Improve direct test coverage and don't rely on other modules' tests for coverage
  • Build a libphobos.a with -cov so that we measure indirect test coverage as well? Would that work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants