You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/02/19 10:07:39 UTC
[GitHub] [commons-lang] lelmarir opened a new pull request #494: LANG-1518 -
fix searchSupers for generic classes
lelmarir opened a new pull request #494: LANG-1518 - fix searchSupers for generic classes
URL: https://github.com/apache/commons-lang/pull/494
https://issues.apache.org/jira/browse/LANG-1518
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] garydgregory commented on issue #494: LANG-1518 -
fix searchSupers for generic classes
Posted by GitBox <gi...@apache.org>.
garydgregory commented on issue #494: LANG-1518 - fix searchSupers for generic classes
URL: https://github.com/apache/commons-lang/pull/494#issuecomment-589694911
- Install Apache Subversion
- Run `mvn clean install site -P jacoco` (I had missed the install step.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] aherbert commented on issue #494: LANG-1518 - fix
searchSupers for generic classes
Posted by GitBox <gi...@apache.org>.
aherbert commented on issue #494: LANG-1518 - fix searchSupers for generic classes
URL: https://github.com/apache/commons-lang/pull/494#issuecomment-589830427
Yes, I think this is just a glitch in how coveralls chooses to report the overall coverage (by using the first build output if there are multiple builds submitting coverage).
You don't need to run the site build which requires svn. You can run the `jacoco:report` goal to report on the results of the `test` goal:
```
mvn test jacoco:report
```
The jacoco profile is active by default (due to the presence of `src/site/resources/profile.jacoco`).
The output will be in the same place as for a site build:
```
target/site/jacoco/index.html
```
Then find your MethodUtils class in the report and check the method you changed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] coveralls commented on issue #494: LANG-1518 - fix
searchSupers for generic classes
Posted by GitBox <gi...@apache.org>.
coveralls commented on issue #494: LANG-1518 - fix searchSupers for generic classes
URL: https://github.com/apache/commons-lang/pull/494#issuecomment-588828142
[](https://coveralls.io/builds/28848609)
Coverage decreased (-0.007%) to 95.095% when pulling **5570791e4438b986c448f3e0f1cdaba1ef4d3adb on lelmarir:LANG-1518** into **eb8d069089364e396e37ed5273cf7710e41eb06d on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] garydgregory merged pull request #494: [LANG-1518]
MethodUtils.getAnnotation() with searchSupers = true does not work if super
is generic
Posted by GitBox <gi...@apache.org>.
garydgregory merged pull request #494: [LANG-1518] MethodUtils.getAnnotation() with searchSupers = true does not work if super is generic
URL: https://github.com/apache/commons-lang/pull/494
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] lelmarir commented on a change in pull request #494:
LANG-1518 - fix searchSupers for generic classes
Posted by GitBox <gi...@apache.org>.
lelmarir commented on a change in pull request #494: LANG-1518 - fix searchSupers for generic classes
URL: https://github.com/apache/commons-lang/pull/494#discussion_r381879283
##########
File path: src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java
##########
@@ -974,17 +974,13 @@ private static int distance(final Class<?>[] classArray, final Class<?>[] toClas
final Class<?> mcls = method.getDeclaringClass();
final List<Class<?>> classes = getAllSuperclassesAndInterfaces(mcls);
for (final Class<?> acls : classes) {
- Method equivalentMethod;
- try {
- equivalentMethod = (ignoreAccess ? acls.getDeclaredMethod(method.getName(), method.getParameterTypes())
- : acls.getMethod(method.getName(), method.getParameterTypes()));
- } catch (final NoSuchMethodException e) {
- // if not found, just keep searching
- continue;
- }
- annotation = equivalentMethod.getAnnotation(annotationCls);
- if (annotation != null) {
- break;
+ Method equivalentMethod = (ignoreAccess ? MethodUtils.getMatchingMethod(acls, method.getName(), method.getParameterTypes())
+ : MethodUtils.getMatchingAccessibleMethod(acls, method.getName(), method.getParameterTypes()));
+ if(equivalentMethod != null) {
Review comment:
i've fixed this one
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] lelmarir commented on issue #494: [LANG-1518]
MethodUtils.getAnnotation() with searchSupers = true does not work if super
is generic
Posted by GitBox <gi...@apache.org>.
lelmarir commented on issue #494: [LANG-1518] MethodUtils.getAnnotation() with searchSupers = true does not work if super is generic
URL: https://github.com/apache/commons-lang/pull/494#issuecomment-590298982
> Yes, I think this is just a glitch in how coveralls chooses to report the overall coverage (by using the first build output if there are multiple builds submitting coverage).
>
> ```
> mvn test jacoco:report
> ```
100% coverage on the method
@aherbert thanks you
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] aherbert commented on issue #494: LANG-1518 - fix
searchSupers for generic classes
Posted by GitBox <gi...@apache.org>.
aherbert commented on issue #494: LANG-1518 - fix searchSupers for generic classes
URL: https://github.com/apache/commons-lang/pull/494#issuecomment-589325241
I think it is a problem with randomness in the test suite and Coveralls choice of summary reporting.
On investigation I think Coveralls is not showing where the drop in coverage is because there are 2 builds and it has created a combined report for coverage but uses only 1 job to report the overall coverage score. It seems to be using the top job (1956.1) to report overall coverage but this has lower coverage than job 1956.4.
The github diff shows 10 actual lines changed to 6. Coveralls thinks you are covering 6/6 new lines. It also thinks the file MethodUtils dropped 2 relevant lines and dropped coverage by 2. So coverage is the same for what is left. Visually the diff looks fine and the summary of changes supports this. So what is different?
You can see what coveralls is comparing against from the Coveralls site by clicking the PR - base link in the top left:
```
PR - base:
14737 of 15496 relevant lines covered (95.1%)
Lines | Relevant | Covered | Missed
MethodUtils 1034 | 251 | 237 | 14
new:
14734 of 15494 relevant lines covered (95.09%)
MethodUtils 1030 (-4) | 249 (-2) | 235 (-2) | 14
```
So overall you removed 2 lines from the codebase but coverage dropped 3. So you get a coverage dropped score. Coveralls is not showing where this line is in the UI. It should show another location which has dropped 1 but it does not.
My guess is that the report of line coverage is a combined report from both builds but the total coverage is taken from just one job. In the PR base both jobs have the same coverage. In your PR the top jobs has lower coverage. I am not familiar with the test suite but this could be due to some stochastic tests that do not always hit the same code points.
I've just scrolled back through builds using the links in Coveralls. Looking back this happened on build 1939 too which has a temp drop in coverage due to the first build job having a lower coverage than the other. A few more back and build 1936 has a lower coverage for the second test but Coveralls does not report this as a drop because the first build is higher. This inconsistency is about 1 in 3 coveralls reports. So there is a test in the test suite that does not hit the same coverage 1 in 6 times. Finding this test is not possible through the Coveralls UI combined report. You'd have to run coveralls locally a few dozen times and see where coverage randomly varies. That is a task for another day.
This is one reason to try and write tests that hit all coverage and then duplicate tests with random data to stress the code. Randomness should not be used to sporadically hit coverage.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] lelmarir edited a comment on issue #494: LANG-1518 -
fix searchSupers for generic classes
Posted by GitBox <gi...@apache.org>.
lelmarir edited a comment on issue #494: LANG-1518 - fix searchSupers for generic classes
URL: https://github.com/apache/commons-lang/pull/494#issuecomment-589563581
> I think it is a problem with randomness in the test suite and Coveralls choice of summary reporting.
So it is a "bug" in Coveralls, and this pull request is good to go?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] garydgregory commented on issue #494: LANG-1518 -
fix searchSupers for generic classes
Posted by GitBox <gi...@apache.org>.
garydgregory commented on issue #494: LANG-1518 - fix searchSupers for generic classes
URL: https://github.com/apache/commons-lang/pull/494#issuecomment-589688459
Run `mvn clean site -P jacoco` and make sure your code gets 100% coverage (or as close to it as possible.) The Jacoco report will be linked from the Reports section in `target\site`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] lelmarir commented on issue #494: LANG-1518 - fix
searchSupers for generic classes
Posted by GitBox <gi...@apache.org>.
lelmarir commented on issue #494: LANG-1518 - fix searchSupers for generic classes
URL: https://github.com/apache/commons-lang/pull/494#issuecomment-589689995
> Run `mvn clean site -P jacoco` and make sure your code gets 100% coverage (or as close to it as possible.) The Jacoco report will be linked from the Reports section in `target\site`.
I get this:
> [ERROR] Failed to execute goal org.apache.maven.plugins:maven-antrun-plugin:1.8:run (prepare-checkout) on project commons-lang3: An Ant BuildException has occured: Execute failed: java.io.IOException: Cannot run program "svn": CreateProcess error=2, Impossibile trovare il file specificato
> [ERROR] around Ant part ...<exec executable="svn">... @ 4:26 in C:\Users\mpreti\Desktop\commons-lang\target\antrun\build-main.xml
do i really need to install svn? (i can't)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] lelmarir commented on issue #494: LANG-1518 - fix
searchSupers for generic classes
Posted by GitBox <gi...@apache.org>.
lelmarir commented on issue #494: LANG-1518 - fix searchSupers for generic classes
URL: https://github.com/apache/commons-lang/pull/494#issuecomment-589563581
> I think it is a problem with randomness in the test suite and Coveralls choice of summary reporting.
So it is a "bug" in Coveralls, and this pull request is good to go?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] lelmarir commented on issue #494: LANG-1518 - fix
searchSupers for generic classes
Posted by GitBox <gi...@apache.org>.
lelmarir commented on issue #494: LANG-1518 - fix searchSupers for generic classes
URL: https://github.com/apache/commons-lang/pull/494#issuecomment-588842247
> [](https://coveralls.io/builds/28848609)
>
> Coverage decreased (-0.007%) to 95.095% when pulling **[5570791](https://github.com/apache/commons-lang/commit/5570791e4438b986c448f3e0f1cdaba1ef4d3adb) on lelmarir:LANG-1518** into **[eb8d069](https://github.com/apache/commons-lang/commit/eb8d069089364e396e37ed5273cf7710e41eb06d) on apache:master**.
can someone help me on this? Is there a way to know what i'm missing to test?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-lang] kinow commented on a change in pull request #494:
LANG-1518 - fix searchSupers for generic classes
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #494: LANG-1518 - fix searchSupers for generic classes
URL: https://github.com/apache/commons-lang/pull/494#discussion_r381451626
##########
File path: src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java
##########
@@ -974,17 +974,13 @@ private static int distance(final Class<?>[] classArray, final Class<?>[] toClas
final Class<?> mcls = method.getDeclaringClass();
final List<Class<?>> classes = getAllSuperclassesAndInterfaces(mcls);
for (final Class<?> acls : classes) {
- Method equivalentMethod;
- try {
- equivalentMethod = (ignoreAccess ? acls.getDeclaredMethod(method.getName(), method.getParameterTypes())
- : acls.getMethod(method.getName(), method.getParameterTypes()));
- } catch (final NoSuchMethodException e) {
- // if not found, just keep searching
- continue;
- }
- annotation = equivalentMethod.getAnnotation(annotationCls);
- if (annotation != null) {
- break;
+ Method equivalentMethod = (ignoreAccess ? MethodUtils.getMatchingMethod(acls, method.getName(), method.getParameterTypes())
+ : MethodUtils.getMatchingAccessibleMethod(acls, method.getName(), method.getParameterTypes()));
+ if(equivalentMethod != null) {
Review comment:
I think Travis is failing here
```
/checkstyle.xml ruleset.
[ERROR] src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java:[979,17] (whitespace) WhitespaceAfter: 'if' is not followed by whitespace.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services