You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2014/10/10 22:59:23 UTC
Review Request 26574: Handle anonymous inner classes better in
per-class coverage check.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26574/
-----------------------------------------------------------
Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
Bugs: AURORA-822
https://issues.apache.org/jira/browse/AURORA-822
Repository: aurora
Description
-------
`javac` inserts synthetic zero-arg constructors into anonymous classes, which jacoco sometimes detects as covered by tests. This was throwing off the "each class must have _some_ coverage" by considering classes without coverage to actually have some coverage.
I've attempted to fix this by ignoring the default constructor in anonymoous classes, which should be more accurate. One sign that this is working — lots of modules came onto the radar as lacking coverage, which is legitimate.
Diffs
-----
build.gradle 8f7eed0b58a00f8e5e0521512975087d5788b5b6
Diff: https://reviews.apache.org/r/26574/diff/
Testing
-------
./gradlew build -Pq
Thanks,
Bill Farner
Re: Review Request 26574: Handle anonymous inner classes better in
per-class coverage check.
Posted by Bill Farner <wf...@apache.org>.
> On Oct. 10, 2014, 9:20 p.m., Maxim Khutornenko wrote:
> > build.gradle, line 609
> > <https://reviews.apache.org/r/26574/diff/1/?file=717796#file717796line609>
> >
> > I have no idea what is going on here... Any way to make it more readable?
Maybe a few more dollar and at-signs would help? :-P
Added some prose to the comment, let me know if that doesn't help.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26574/#review56225
-----------------------------------------------------------
On Oct. 10, 2014, 9:13 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26574/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 9:13 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
>
>
> Bugs: AURORA-822
> https://issues.apache.org/jira/browse/AURORA-822
>
>
> Repository: aurora
>
>
> Description
> -------
>
> `javac` inserts synthetic zero-arg constructors into anonymous classes, which jacoco sometimes detects as covered by tests. This was throwing off the "each class must have _some_ coverage" by considering classes without coverage to actually have some coverage.
>
> I've attempted to fix this by ignoring the default constructor in anonymoous classes, which should be more accurate. One sign that this is working — lots of modules came onto the radar as lacking coverage, which is legitimate.
>
>
> Diffs
> -----
>
> build.gradle 8f7eed0b58a00f8e5e0521512975087d5788b5b6
>
> Diff: https://reviews.apache.org/r/26574/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> I also verified that i can now cherry-pick the patch from https://reviews.apache.org/r/26469/ and produce a green build. On master, i consistently get a coverage error.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26574: Handle anonymous inner classes better in
per-class coverage check.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26574/#review56225
-----------------------------------------------------------
Ship it!
build.gradle
<https://reviews.apache.org/r/26574/#comment96550>
I have no idea what is going on here... Any way to make it more readable?
- Maxim Khutornenko
On Oct. 10, 2014, 9:13 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26574/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 9:13 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
>
>
> Bugs: AURORA-822
> https://issues.apache.org/jira/browse/AURORA-822
>
>
> Repository: aurora
>
>
> Description
> -------
>
> `javac` inserts synthetic zero-arg constructors into anonymous classes, which jacoco sometimes detects as covered by tests. This was throwing off the "each class must have _some_ coverage" by considering classes without coverage to actually have some coverage.
>
> I've attempted to fix this by ignoring the default constructor in anonymoous classes, which should be more accurate. One sign that this is working — lots of modules came onto the radar as lacking coverage, which is legitimate.
>
>
> Diffs
> -----
>
> build.gradle 8f7eed0b58a00f8e5e0521512975087d5788b5b6
>
> Diff: https://reviews.apache.org/r/26574/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> I also verified that i can now cherry-pick the patch from https://reviews.apache.org/r/26469/ and produce a green build. On master, i consistently get a coverage error.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26574: Handle anonymous inner classes better in
per-class coverage check.
Posted by Kevin Sweeney <ke...@apache.org>.
> On Oct. 10, 2014, 2:43 p.m., Joshua Cohen wrote:
> > This task is getting somewhat complex, do you think it makes sense to externalize it[1], rather than having build.gradle take on more and more logic?
> >
> > [1] http://www.gradle.org/docs/current/userguide/custom_tasks.html
>
> Bill Farner wrote:
> Thanks for the nudge, this crossed my mind as well. Left as a TODO for a rainy day to unblock for now.
+1, I'd also like to nail down the story for sanely managing our build configuration. I've also got a custom PMD rule that I'd like to figure out how to wire in https://gist.github.com/kevints/3638383e1b10f1081c44, but that looks to be as difficult as writing it was.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26574/#review56227
-----------------------------------------------------------
On Oct. 10, 2014, 2:51 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26574/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 2:51 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
>
>
> Bugs: AURORA-822
> https://issues.apache.org/jira/browse/AURORA-822
>
>
> Repository: aurora
>
>
> Description
> -------
>
> `javac` inserts synthetic zero-arg constructors into anonymous classes, which jacoco sometimes detects as covered by tests. This was throwing off the "each class must have _some_ coverage" by considering classes without coverage to actually have some coverage.
>
> I've attempted to fix this by ignoring the default constructor in anonymoous classes, which should be more accurate. One sign that this is working — lots of modules came onto the radar as lacking coverage, which is legitimate.
>
>
> Diffs
> -----
>
> build.gradle 8f7eed0b58a00f8e5e0521512975087d5788b5b6
>
> Diff: https://reviews.apache.org/r/26574/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> I also verified that i can now cherry-pick the patch from https://reviews.apache.org/r/26469/ and produce a green build. On master, i consistently get a coverage error.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26574: Handle anonymous inner classes better in
per-class coverage check.
Posted by Bill Farner <wf...@apache.org>.
> On Oct. 10, 2014, 9:43 p.m., Joshua Cohen wrote:
> > This task is getting somewhat complex, do you think it makes sense to externalize it[1], rather than having build.gradle take on more and more logic?
> >
> > [1] http://www.gradle.org/docs/current/userguide/custom_tasks.html
Thanks for the nudge, this crossed my mind as well. Left as a TODO for a rainy day to unblock for now.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26574/#review56227
-----------------------------------------------------------
On Oct. 10, 2014, 9:41 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26574/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 9:41 p.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-822
> https://issues.apache.org/jira/browse/AURORA-822
>
>
> Repository: aurora
>
>
> Description
> -------
>
> `javac` inserts synthetic zero-arg constructors into anonymous classes, which jacoco sometimes detects as covered by tests. This was throwing off the "each class must have _some_ coverage" by considering classes without coverage to actually have some coverage.
>
> I've attempted to fix this by ignoring the default constructor in anonymoous classes, which should be more accurate. One sign that this is working — lots of modules came onto the radar as lacking coverage, which is legitimate.
>
>
> Diffs
> -----
>
> build.gradle 8f7eed0b58a00f8e5e0521512975087d5788b5b6
>
> Diff: https://reviews.apache.org/r/26574/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> I also verified that i can now cherry-pick the patch from https://reviews.apache.org/r/26469/ and produce a green build. On master, i consistently get a coverage error.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26574: Handle anonymous inner classes better in
per-class coverage check.
Posted by Joshua Cohen <jc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26574/#review56227
-----------------------------------------------------------
This task is getting somewhat complex, do you think it makes sense to externalize it[1], rather than having build.gradle take on more and more logic?
[1] http://www.gradle.org/docs/current/userguide/custom_tasks.html
- Joshua Cohen
On Oct. 10, 2014, 9:41 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26574/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 9:41 p.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-822
> https://issues.apache.org/jira/browse/AURORA-822
>
>
> Repository: aurora
>
>
> Description
> -------
>
> `javac` inserts synthetic zero-arg constructors into anonymous classes, which jacoco sometimes detects as covered by tests. This was throwing off the "each class must have _some_ coverage" by considering classes without coverage to actually have some coverage.
>
> I've attempted to fix this by ignoring the default constructor in anonymoous classes, which should be more accurate. One sign that this is working — lots of modules came onto the radar as lacking coverage, which is legitimate.
>
>
> Diffs
> -----
>
> build.gradle 8f7eed0b58a00f8e5e0521512975087d5788b5b6
>
> Diff: https://reviews.apache.org/r/26574/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> I also verified that i can now cherry-pick the patch from https://reviews.apache.org/r/26469/ and produce a green build. On master, i consistently get a coverage error.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26574: Handle anonymous inner classes better in
per-class coverage check.
Posted by Joshua Cohen <jc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26574/#review56228
-----------------------------------------------------------
Ship it!
I'm fine shipping this, we can have a separate discussion about organizing build.gradle.
- Joshua Cohen
On Oct. 10, 2014, 9:41 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26574/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 9:41 p.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-822
> https://issues.apache.org/jira/browse/AURORA-822
>
>
> Repository: aurora
>
>
> Description
> -------
>
> `javac` inserts synthetic zero-arg constructors into anonymous classes, which jacoco sometimes detects as covered by tests. This was throwing off the "each class must have _some_ coverage" by considering classes without coverage to actually have some coverage.
>
> I've attempted to fix this by ignoring the default constructor in anonymoous classes, which should be more accurate. One sign that this is working — lots of modules came onto the radar as lacking coverage, which is legitimate.
>
>
> Diffs
> -----
>
> build.gradle 8f7eed0b58a00f8e5e0521512975087d5788b5b6
>
> Diff: https://reviews.apache.org/r/26574/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
> I also verified that i can now cherry-pick the patch from https://reviews.apache.org/r/26469/ and produce a green build. On master, i consistently get a coverage error.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26574: Handle anonymous inner classes better in
per-class coverage check.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26574/
-----------------------------------------------------------
(Updated Oct. 10, 2014, 9:51 p.m.)
Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
Bugs: AURORA-822
https://issues.apache.org/jira/browse/AURORA-822
Repository: aurora
Description
-------
`javac` inserts synthetic zero-arg constructors into anonymous classes, which jacoco sometimes detects as covered by tests. This was throwing off the "each class must have _some_ coverage" by considering classes without coverage to actually have some coverage.
I've attempted to fix this by ignoring the default constructor in anonymoous classes, which should be more accurate. One sign that this is working — lots of modules came onto the radar as lacking coverage, which is legitimate.
Diffs (updated)
-----
build.gradle 8f7eed0b58a00f8e5e0521512975087d5788b5b6
Diff: https://reviews.apache.org/r/26574/diff/
Testing
-------
./gradlew build -Pq
I also verified that i can now cherry-pick the patch from https://reviews.apache.org/r/26469/ and produce a green build. On master, i consistently get a coverage error.
Thanks,
Bill Farner
Re: Review Request 26574: Handle anonymous inner classes better in
per-class coverage check.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26574/
-----------------------------------------------------------
(Updated Oct. 10, 2014, 9:41 p.m.)
Review request for Aurora and Maxim Khutornenko.
Changes
-------
jcohen: removing you to get this cleaned up and get your change in. Happy to address any comments async.
Bugs: AURORA-822
https://issues.apache.org/jira/browse/AURORA-822
Repository: aurora
Description
-------
`javac` inserts synthetic zero-arg constructors into anonymous classes, which jacoco sometimes detects as covered by tests. This was throwing off the "each class must have _some_ coverage" by considering classes without coverage to actually have some coverage.
I've attempted to fix this by ignoring the default constructor in anonymoous classes, which should be more accurate. One sign that this is working — lots of modules came onto the radar as lacking coverage, which is legitimate.
Diffs
-----
build.gradle 8f7eed0b58a00f8e5e0521512975087d5788b5b6
Diff: https://reviews.apache.org/r/26574/diff/
Testing
-------
./gradlew build -Pq
I also verified that i can now cherry-pick the patch from https://reviews.apache.org/r/26469/ and produce a green build. On master, i consistently get a coverage error.
Thanks,
Bill Farner
Re: Review Request 26574: Handle anonymous inner classes better in
per-class coverage check.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26574/
-----------------------------------------------------------
(Updated Oct. 10, 2014, 9:33 p.m.)
Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
Bugs: AURORA-822
https://issues.apache.org/jira/browse/AURORA-822
Repository: aurora
Description
-------
`javac` inserts synthetic zero-arg constructors into anonymous classes, which jacoco sometimes detects as covered by tests. This was throwing off the "each class must have _some_ coverage" by considering classes without coverage to actually have some coverage.
I've attempted to fix this by ignoring the default constructor in anonymoous classes, which should be more accurate. One sign that this is working — lots of modules came onto the radar as lacking coverage, which is legitimate.
Diffs (updated)
-----
build.gradle 8f7eed0b58a00f8e5e0521512975087d5788b5b6
Diff: https://reviews.apache.org/r/26574/diff/
Testing
-------
./gradlew build -Pq
I also verified that i can now cherry-pick the patch from https://reviews.apache.org/r/26469/ and produce a green build. On master, i consistently get a coverage error.
Thanks,
Bill Farner
Re: Review Request 26574: Handle anonymous inner classes better in
per-class coverage check.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26574/
-----------------------------------------------------------
(Updated Oct. 10, 2014, 9:13 p.m.)
Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
Bugs: AURORA-822
https://issues.apache.org/jira/browse/AURORA-822
Repository: aurora
Description
-------
`javac` inserts synthetic zero-arg constructors into anonymous classes, which jacoco sometimes detects as covered by tests. This was throwing off the "each class must have _some_ coverage" by considering classes without coverage to actually have some coverage.
I've attempted to fix this by ignoring the default constructor in anonymoous classes, which should be more accurate. One sign that this is working — lots of modules came onto the radar as lacking coverage, which is legitimate.
Diffs
-----
build.gradle 8f7eed0b58a00f8e5e0521512975087d5788b5b6
Diff: https://reviews.apache.org/r/26574/diff/
Testing (updated)
-------
./gradlew build -Pq
I also verified that i can now cherry-pick the patch from https://reviews.apache.org/r/26469/ and produce a green build. On master, i consistently get a coverage error.
Thanks,
Bill Farner