-
Notifications
You must be signed in to change notification settings - Fork 174
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
[SCM-989] Tests fail if svn and/or git are not installed #149
Conversation
@nielsbasjes Have a look. |
if ( !ScmTestCase.isSystemCmd( SvnScmTestUtils.SVN_COMMAND_LINE ) ) | ||
{ | ||
ScmTestCase.printSystemCmdUnavail( SvnScmTestUtils.SVN_COMMAND_LINE, getName() ); | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I see this code fragment duplicated many times. Should we make this into a method that returns a boolean?
- If svn is missing then this test will pass. Now tests can pass, fail or be skipped. In many of the cases here I would have chosen to mark them as skipped instead of (as I read the code) passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a problem, I have a local branch which uses your new code:
diff --git a/maven-scm-plugin/src/test/java/org/apache/maven/scm/plugin/BranchMojoTest.java b/maven-scm-plugin/src/test/java/org/apache/maven/scm/plugin/BranchMojoTest.java
index ef788478..33d27e7f 100644
--- a/maven-scm-plugin/src/test/java/org/apache/maven/scm/plugin/BranchMojoTest.java
+++ b/maven-scm-plugin/src/test/java/org/apache/maven/scm/plugin/BranchMojoTest.java
@@ -27,6 +27,9 @@
import java.io.File;
+import static org.apache.maven.scm.provider.svn.SvnScmTestUtils.SVN_COMMAND_LINE;
+import static org.apache.maven.scm.provider.svn.SvnScmTestUtils.SVNADMIN_COMMAND_LINE;
+
/**
* @author <a href="mailto:[email protected]">Emmanuel Venisse</a>
*
@@ -51,11 +54,7 @@ protected void setUp()
FileUtils.forceDelete( repository );
- if ( !ScmTestCase.isSystemCmd( SvnScmTestUtils.SVNADMIN_COMMAND_LINE ) )
- {
- ScmTestCase.printSystemCmdUnavail( SvnScmTestUtils.SVNADMIN_COMMAND_LINE, "setUp" );
- return;
- }
+ ScmTestCase.checkScmPresence( SVNADMIN_COMMAND_LINE );
SvnScmTestUtils.initializeRepository( repository );
But all of these changes fail with AssertionViolationException
. There is some old code in Plugin Testing which does not work with Assume
. Therefore, I'd like to move on with this and analyze it in a subsequent ticket: https://issues.apache.org/jira/projects/SCM/issues/SCM-939
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then I fully agree with your current code.
@nielsbasjes Here is the patch, watch the failure. Consider the code in mojo tests as intermediate. |
This closes #149