Skip to content

Reactivate coverage tracking for dartdoc via coveralls #1869

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

Merged
merged 19 commits into from
Dec 14, 2018
Merged

Conversation

jcollins-g
Copy link
Contributor

@jcollins-g jcollins-g commented Dec 12, 2018

It's been more than 3 years since coverage worked on dartdoc, but with this PR coverage tracking returns!

https://coveralls.io/github/dart-lang/dartdoc?branch=coveralls for the coverage result for this branch, with this PR. It compares our coverage to what it was on master, here: https://coveralls.io/builds/11445981

That was the last update from August 10, 2015.

To make this work some hacks were needed. dart-lang/sdk#31747 meant we had to avoid use of 'exit()' in dart, and dart-lang/tools#448 meant we had to fork a modified version of the 'format_coverage.dart' binary into dartdoc to get around the type error. I don't use dart_coveralls here because I'd either have to run the tests twice or lose parallelization in dartdoc's custom test runner.

@googlebot googlebot added the cla: yes Google CLA check succeeded. label Dec 12, 2018
@jcollins-g jcollins-g changed the title WIP: Try to turn coveralls on again for dartdoc Reactivate coverage tracking for dartdoc via coveralls Dec 13, 2018
@jcollins-g jcollins-g requested review from pq and removed request for keertip December 13, 2018 20:03
Copy link
Member

@pq pq left a comment

Choose a reason for hiding this comment

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

💎 Awesome! 💎

@pq
Copy link
Member

pq commented Dec 14, 2018

To make this work some hacks were needed.

Do you feel like there would be any value documenting this somewhere? Or is dartdoc too much of it's own odd thing?

@jcollins-g
Copy link
Contributor Author

I don't think dartdoc is that odd for a complex project with many tests; it's just that 1) we don't have good support for layering things like coverage runs into the default test runner and 2) default test runners don't easily support parallel execution. So the bits we have only work for trivial cases, or cases where you don't mind spending a lot of cloud resources, or you have to roll your own like what dartdoc does.

Maybe it would be nice if these issues were addressed in a more comprehensive way, though.

Copy link
Member

@pq pq left a comment

Choose a reason for hiding this comment

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

image

@jcollins-g jcollins-g merged commit 4a14687 into master Dec 14, 2018
@jcollins-g jcollins-g deleted the coveralls branch December 14, 2018 18:16
@@ -34,6 +34,7 @@ dev_dependencies:
build: ^1.0.1
build_runner: ^1.0.0
build_version: ^1.0.0
coverage: any
Copy link
Member

Choose a reason for hiding this comment

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

Are you intending to let this float?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

@@ -0,0 +1,232 @@
// Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file
Copy link
Member

Choose a reason for hiding this comment

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

This is an alternative front end to package:coverage (to better support merging results from multiple runs)? If so, perhaps add a file or class comment to that effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is just a temporary fork as I mentioned above. with the next release of coverage, i will constrain version and eliminate the fork.

Copy link
Member

Choose a reason for hiding this comment

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

gotcha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants