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/14 01:01:50 UTC
Review Request 26662: Move coverage report check to buildSrc/
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26662/
-----------------------------------------------------------
Review request for Aurora, Joshua Cohen and Kevin Sweeney.
Bugs: AURORA-833
https://issues.apache.org/jira/browse/AURORA-833
Repository: aurora
Description
-------
Making good on a TODO from https://reviews.apache.org/r/26574/
Diffs
-----
.gitignore ad4206d179b46f1269fd58c62de9e6e62f4b8738
build.gradle c200746ff9ce2d7ff973c22764d9413bf64636e7
buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy PRE-CREATION
config/legacy_untested_classes.txt PRE-CREATION
Diff: https://reviews.apache.org/r/26662/diff/
Testing
-------
./gradlew build -Pq
Thanks,
Bill Farner
Re: Review Request 26662: Move coverage report check to buildSrc/
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26662/#review56472
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Sweeney
On Oct. 13, 2014, 4:36 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26662/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2014, 4:36 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
>
>
> Bugs: AURORA-833
> https://issues.apache.org/jira/browse/AURORA-833
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Making good on a TODO from https://reviews.apache.org/r/26574/
>
>
> Diffs
> -----
>
> .gitignore ad4206d179b46f1269fd58c62de9e6e62f4b8738
> build.gradle c200746ff9ce2d7ff973c22764d9413bf64636e7
> buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy PRE-CREATION
> config/legacy_untested_classes.txt PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26662/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26662: Move coverage report check to buildSrc/
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26662/
-----------------------------------------------------------
(Updated Oct. 13, 2014, 11:36 p.m.)
Review request for Aurora, Joshua Cohen and Kevin Sweeney.
Bugs: AURORA-833
https://issues.apache.org/jira/browse/AURORA-833
Repository: aurora
Description
-------
Making good on a TODO from https://reviews.apache.org/r/26574/
Diffs (updated)
-----
.gitignore ad4206d179b46f1269fd58c62de9e6e62f4b8738
build.gradle c200746ff9ce2d7ff973c22764d9413bf64636e7
buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy PRE-CREATION
config/legacy_untested_classes.txt PRE-CREATION
Diff: https://reviews.apache.org/r/26662/diff/
Testing
-------
./gradlew build -Pq
Thanks,
Bill Farner
Re: Review Request 26662: Move coverage report check to buildSrc/
Posted by Bill Farner <wf...@apache.org>.
> On Oct. 13, 2014, 11:08 p.m., Kevin Sweeney wrote:
> > buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy, line 18
> > <https://reviews.apache.org/r/26662/diff/2/?file=719791#file719791line18>
> >
> > typo in analyzes
Fixed.
> On Oct. 13, 2014, 11:08 p.m., Kevin Sweeney wrote:
> > buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy, line 61
> > <https://reviews.apache.org/r/26662/diff/2/?file=719791#file719791line61>
> >
> > typo in "anonymous"
Fixed.
> On Oct. 13, 2014, 11:08 p.m., Kevin Sweeney wrote:
> > buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy, line 53
> > <https://reviews.apache.org/r/26662/diff/2/?file=719791#file719791line53>
> >
> > I think you want to throw a GradleException here and below
Went with `TaskExecutionException`, LMK if you disagree.
> A TaskExecutionException is thrown when a task fails to execute successfully.
http://www.gradle.org/docs/current/javadoc/org/gradle/api/tasks/TaskExecutionException.html
> On Oct. 13, 2014, 11:08 p.m., Kevin Sweeney wrote:
> > build.gradle, line 642
> > <https://reviews.apache.org/r/26662/diff/2/?file=719790#file719790line642>
> >
> > Does this slow down the build at all (you're now doing I/O in configure?)
My unscientific efforts say: not in any way i can measure.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26662/#review56459
-----------------------------------------------------------
On Oct. 13, 2014, 11:03 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26662/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2014, 11:03 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
>
>
> Bugs: AURORA-833
> https://issues.apache.org/jira/browse/AURORA-833
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Making good on a TODO from https://reviews.apache.org/r/26574/
>
>
> Diffs
> -----
>
> .gitignore ad4206d179b46f1269fd58c62de9e6e62f4b8738
> build.gradle c200746ff9ce2d7ff973c22764d9413bf64636e7
> buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy PRE-CREATION
> config/legacy_untested_classes.txt PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26662/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26662: Move coverage report check to buildSrc/
Posted by Bill Farner <wf...@apache.org>.
> On Oct. 13, 2014, 11:08 p.m., Kevin Sweeney wrote:
> > buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy, line 53
> > <https://reviews.apache.org/r/26662/diff/2/?file=719791#file719791line53>
> >
> > I think you want to throw a GradleException here and below
>
> Bill Farner wrote:
> Went with `TaskExecutionException`, LMK if you disagree.
>
> > A TaskExecutionException is thrown when a task fails to execute successfully.
>
> http://www.gradle.org/docs/current/javadoc/org/gradle/api/tasks/TaskExecutionException.html
Scratch that - `TaskExecutionException` requires a Throwable cause. Backed out to `GradleException`.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26662/#review56459
-----------------------------------------------------------
On Oct. 13, 2014, 11:03 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26662/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2014, 11:03 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
>
>
> Bugs: AURORA-833
> https://issues.apache.org/jira/browse/AURORA-833
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Making good on a TODO from https://reviews.apache.org/r/26574/
>
>
> Diffs
> -----
>
> .gitignore ad4206d179b46f1269fd58c62de9e6e62f4b8738
> build.gradle c200746ff9ce2d7ff973c22764d9413bf64636e7
> buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy PRE-CREATION
> config/legacy_untested_classes.txt PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26662/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26662: Move coverage report check to buildSrc/
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26662/#review56459
-----------------------------------------------------------
build.gradle
<https://reviews.apache.org/r/26662/#comment96805>
Does this slow down the build at all (you're now doing I/O in configure?)
buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy
<https://reviews.apache.org/r/26662/#comment96807>
typo in analyzes
buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy
<https://reviews.apache.org/r/26662/#comment96801>
I think you want to throw a GradleException here and below
buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy
<https://reviews.apache.org/r/26662/#comment96804>
typo in "anonymous"
- Kevin Sweeney
On Oct. 13, 2014, 4:03 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26662/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2014, 4:03 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
>
>
> Bugs: AURORA-833
> https://issues.apache.org/jira/browse/AURORA-833
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Making good on a TODO from https://reviews.apache.org/r/26574/
>
>
> Diffs
> -----
>
> .gitignore ad4206d179b46f1269fd58c62de9e6e62f4b8738
> build.gradle c200746ff9ce2d7ff973c22764d9413bf64636e7
> buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy PRE-CREATION
> config/legacy_untested_classes.txt PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26662/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26662: Move coverage report check to buildSrc/
Posted by Kevin Sweeney <ke...@apache.org>.
> On Oct. 13, 2014, 4:23 p.m., Joshua Cohen wrote:
> > buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy, line 53
> > <https://reviews.apache.org/r/26662/diff/2/?file=719791#file719791line53>
> >
> > I'm not super familiar w/ groovy, any reason why we can't just inline the computeCoverage(...) call here (and below)?
>
> Bill Farner wrote:
> This was a nuance in the way the assertion error would show - with inlining, you wouldn't see the actual value for some reason. So you would see "0.9" instead of "0.89 > 0.9".
>
> Bill Farner wrote:
> I've gone ahead and inlined now, though, since in retrospect i don't think we really care about the absolute value; and it's easy enough to get from the coverage report HTML.
Technically you could write it in Java :)
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26662/#review56466
-----------------------------------------------------------
On Oct. 13, 2014, 4:36 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26662/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2014, 4:36 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
>
>
> Bugs: AURORA-833
> https://issues.apache.org/jira/browse/AURORA-833
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Making good on a TODO from https://reviews.apache.org/r/26574/
>
>
> Diffs
> -----
>
> .gitignore ad4206d179b46f1269fd58c62de9e6e62f4b8738
> build.gradle c200746ff9ce2d7ff973c22764d9413bf64636e7
> buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy PRE-CREATION
> config/legacy_untested_classes.txt PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26662/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26662: Move coverage report check to buildSrc/
Posted by Bill Farner <wf...@apache.org>.
> On Oct. 13, 2014, 11:23 p.m., Joshua Cohen wrote:
> > buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy, line 53
> > <https://reviews.apache.org/r/26662/diff/2/?file=719791#file719791line53>
> >
> > I'm not super familiar w/ groovy, any reason why we can't just inline the computeCoverage(...) call here (and below)?
This was a nuance in the way the assertion error would show - with inlining, you wouldn't see the actual value for some reason. So you would see "0.9" instead of "0.89 > 0.9".
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26662/#review56466
-----------------------------------------------------------
On Oct. 13, 2014, 11:03 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26662/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2014, 11:03 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
>
>
> Bugs: AURORA-833
> https://issues.apache.org/jira/browse/AURORA-833
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Making good on a TODO from https://reviews.apache.org/r/26574/
>
>
> Diffs
> -----
>
> .gitignore ad4206d179b46f1269fd58c62de9e6e62f4b8738
> build.gradle c200746ff9ce2d7ff973c22764d9413bf64636e7
> buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy PRE-CREATION
> config/legacy_untested_classes.txt PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26662/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26662: Move coverage report check to buildSrc/
Posted by Bill Farner <wf...@apache.org>.
> On Oct. 13, 2014, 11:23 p.m., Joshua Cohen wrote:
> > buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy, line 53
> > <https://reviews.apache.org/r/26662/diff/2/?file=719791#file719791line53>
> >
> > I'm not super familiar w/ groovy, any reason why we can't just inline the computeCoverage(...) call here (and below)?
>
> Bill Farner wrote:
> This was a nuance in the way the assertion error would show - with inlining, you wouldn't see the actual value for some reason. So you would see "0.9" instead of "0.89 > 0.9".
I've gone ahead and inlined now, though, since in retrospect i don't think we really care about the absolute value; and it's easy enough to get from the coverage report HTML.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26662/#review56466
-----------------------------------------------------------
On Oct. 13, 2014, 11:03 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26662/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2014, 11:03 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
>
>
> Bugs: AURORA-833
> https://issues.apache.org/jira/browse/AURORA-833
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Making good on a TODO from https://reviews.apache.org/r/26574/
>
>
> Diffs
> -----
>
> .gitignore ad4206d179b46f1269fd58c62de9e6e62f4b8738
> build.gradle c200746ff9ce2d7ff973c22764d9413bf64636e7
> buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy PRE-CREATION
> config/legacy_untested_classes.txt PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26662/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26662: Move coverage report check to buildSrc/
Posted by Joshua Cohen <jc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26662/#review56466
-----------------------------------------------------------
Ship it!
Looks good to me modulo Kevin's comments.
buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy
<https://reviews.apache.org/r/26662/#comment96820>
I'm not super familiar w/ groovy, any reason why we can't just inline the computeCoverage(...) call here (and below)?
- Joshua Cohen
On Oct. 13, 2014, 11:03 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26662/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2014, 11:03 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
>
>
> Bugs: AURORA-833
> https://issues.apache.org/jira/browse/AURORA-833
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Making good on a TODO from https://reviews.apache.org/r/26574/
>
>
> Diffs
> -----
>
> .gitignore ad4206d179b46f1269fd58c62de9e6e62f4b8738
> build.gradle c200746ff9ce2d7ff973c22764d9413bf64636e7
> buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy PRE-CREATION
> config/legacy_untested_classes.txt PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26662/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26662: Move coverage report check to buildSrc/
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26662/
-----------------------------------------------------------
(Updated Oct. 13, 2014, 11:03 p.m.)
Review request for Aurora, Joshua Cohen and Kevin Sweeney.
Changes
-------
Added license header.
Bugs: AURORA-833
https://issues.apache.org/jira/browse/AURORA-833
Repository: aurora
Description
-------
Making good on a TODO from https://reviews.apache.org/r/26574/
Diffs (updated)
-----
.gitignore ad4206d179b46f1269fd58c62de9e6e62f4b8738
build.gradle c200746ff9ce2d7ff973c22764d9413bf64636e7
buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy PRE-CREATION
config/legacy_untested_classes.txt PRE-CREATION
Diff: https://reviews.apache.org/r/26662/diff/
Testing
-------
./gradlew build -Pq
Thanks,
Bill Farner