You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/04/28 10:53:38 UTC
[GitHub] [maven-surefire] laeubi opened a new pull request #349: Fix SUREFIRE-1910 - Missleading error message when using -Dtest=....
laeubi opened a new pull request #349:
URL: https://github.com/apache/maven-surefire/pull/349
See https://issues.apache.org/jira/browse/SUREFIRE-1910
--
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
[GitHub] [maven-surefire] Tibor17 commented on pull request #349: Fix SUREFIRE-1910 - Missleading error message when using -Dtest=....
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #349:
URL: https://github.com/apache/maven-surefire/pull/349#issuecomment-828663502
@laeubi
The original code I was pointing to was right.
--
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
[GitHub] [maven-surefire] laeubi commented on pull request #349: Fix SUREFIRE-1910 - Missleading error message when using -Dtest=....
Posted by GitBox <gi...@apache.org>.
laeubi commented on pull request #349:
URL: https://github.com/apache/maven-surefire/pull/349#issuecomment-828650797
Well its at least confusing (for me) but I'm sure there is a good reason for it :-)
I think the NULL checks are there to allow sub classes to either explicit specify a value (true/false) or to specify 'no value, use any sensible default' (null).
Your change changes this logic a bit, currently it behaves (if I understand it right) that if i specify -Dtest... (isSpecificTestSpecified() returns true) the default changes to error if no test is found because it is assumed that I wan't to run at least one 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
[GitHub] [maven-surefire] Tibor17 commented on pull request #349: Fix SUREFIRE-1910 - Missleading error message when using -Dtest=....
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #349:
URL: https://github.com/apache/maven-surefire/pull/349#issuecomment-828669027
@laeubi
I think the message is not a problem. The problem is that `failIfNoTests` is not set to `false` by default, and it should be! If it was set to `false`, the method `getEffectiveFailIfNoTests` would return false for existing tests. For non-existing tests where the `failIfNoSpecifiedTests` should be set to `true` by default (currently null - bad) should also return `false`. But the problem is that these two `false` have different meaning and therefore this method should return an enum with one successful return and two negative returns for errors. Then I would agree with you @laeubi that the message should be more meaningful but upon the enum values.
--
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
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #349: Fix SUREFIRE-1910 - Missleading error message when using -Dtest=....
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #349:
URL: https://github.com/apache/maven-surefire/pull/349#issuecomment-828669027
@laeubi
I think the message is not a root cause. It is also a problem but it is not root cause. The problem is that `failIfNoTests` is not set to `false` by default, and it should be! If it was set to `false`, the method `getEffectiveFailIfNoTests` would return false for existing tests. For non-existing tests where the `failIfNoSpecifiedTests` should be set to `true` by default (currently null - bad) should also return `false`. But the problem is that these two `false` have different meaning and therefore this method should return an enum with one successful return and two negative returns for errors. Then I would agree with you @laeubi that the message should be more meaningful but upon the enum values.
--
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
[GitHub] [maven-surefire] Tibor17 closed pull request #349: Fix SUREFIRE-1910 - Missleading error message when using -Dtest=....
Posted by GitBox <gi...@apache.org>.
Tibor17 closed pull request #349:
URL: https://github.com/apache/maven-surefire/pull/349
--
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
[GitHub] [maven-surefire] Tibor17 commented on pull request #349: Fix SUREFIRE-1910 - Missleading error message when using -Dtest=....
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #349:
URL: https://github.com/apache/maven-surefire/pull/349#issuecomment-841647619
Closing as resolved in pr #350 .
--
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
[GitHub] [maven-surefire] Tibor17 commented on pull request #349: Fix SUREFIRE-1910 - Missleading error message when using -Dtest=....
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #349:
URL: https://github.com/apache/maven-surefire/pull/349#issuecomment-837508561
@laeubi
Pls let me know what you think of the fix in a new [branch](https://github.com/apache/maven-surefire/tree/SUREFIRE-1910).
--
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
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #349: Fix SUREFIRE-1910 - Missleading error message when using -Dtest=....
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #349:
URL: https://github.com/apache/maven-surefire/pull/349#issuecomment-828669027
@laeubi
I think the message is not a root cause. It is also a problem but it is not root cause. The problem is that `failIfNoTests` is not set to `false` by default, and it should be! If it was set to `false`, the method `getEffectiveFailIfNoTests` would return false for existing tests. For non-existing tests where the `failIfNoSpecifiedTests` should be set to `true` by default (currently null - bad) should also return `false`. But the problem is that these two `false` have different meaning and therefore this method should return an enum with one successful and two negative values for errors. Then I would agree with you @laeubi that the message should be more meaningful but upon the enum values.
--
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
[GitHub] [maven-surefire] Tibor17 commented on pull request #349: Fix SUREFIRE-1910 - Missleading error message when using -Dtest=....
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #349:
URL: https://github.com/apache/maven-surefire/pull/349#issuecomment-828643297
IMO this method is illogical:
```
private boolean getEffectiveFailIfNoTests()
{
if ( isSpecificTestSpecified() )
{
if ( getFailIfNoSpecifiedTests() != null )
{
return getFailIfNoSpecifiedTests();
}
else if ( getFailIfNoTests() != null )
{
return getFailIfNoTests();
}
else
{
return true;
}
}
else
{
return getFailIfNoTests() != null && getFailIfNoTests();
}
}
```
and should be changed to something like this
```
private boolean getEffectiveFailIfNoTests()
{
if ( isSpecificTestSpecified() )
{
if ( getFailIfNoTests() != null )
{
return getFailIfNoTests();
}
else
{
return true;
}
}
else
{
if ( getFailIfNoSpecifiedTests() != null )
{
return getFailIfNoSpecifiedTests();
}
return getFailIfNoTests() != null && getFailIfNoTests();
}
}
```
WDYT?
The methods `getFailIfNoSpecifiedTests` and `getFailIfNoTests` are also wrong and they should never return NULL, see the Javadoc about default values.
--
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
[GitHub] [maven-surefire] laeubi commented on pull request #349: Fix SUREFIRE-1910 - Missleading error message when using -Dtest=....
Posted by GitBox <gi...@apache.org>.
laeubi commented on pull request #349:
URL: https://github.com/apache/maven-surefire/pull/349#issuecomment-888910251
@Tibor17 it seems the issue is not resolved completely see https://github.com/eclipse/tycho/issues/192#issuecomment-888870338 the message is much clearer but still a build fails just because a pattern is given (what should not happen if I have understand you right...)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org