[GitHub] [zookeeper] ctubbsii commented on a diff in pull request #2038: ZOOKEEPER-4726: Upgrade snappy-java version to v1.1.10.3

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii commented on code in PR #2038:
URL: https://github.com/apache/zookeeper/pull/2038#discussion_r1303870923


##########
Jenkinsfile-s390x:
##########
@@ -48,7 +48,7 @@ pipeline {
                 stage('BuildAndTest') {
                     steps {
                         sh "git clean -fxd"
-                        sh "mvn verify spotbugs:check checkstyle:check -Pfull-build -Dsurefire-forkcount=4"
+                        sh "mvn verify spotbugs:check checkstyle:check -Pfull-build -Dsurefire-forkcount=4 -Dtest=\!ClientSSLTest -DfailIfNoTests=false"

Review Comment:
   Thinking about this a bit more, I think you actually shouldn't skip the test here. Instead, do it in JUnit. In the ClientSSLTest, you can add:
   
   ```java
       Assumptions.assumeFalse(System.getProperty("os.arch").contains("s390x"), "Test cannot run for s390x because ...").
   ```
   
   If you add something like that to the top of a test case, JUnit will not only happily skip the test, but it will also helpfully print out an explanation of why the test was skipped. Keeping the exclusion close to the test case itself is better than writing the exclusion into the Jenkinsfile.
   
   Also, it's not even clear that this is a good pattern to use... as it will try to execute any utility classes as test classes, in addition to the default pattern of `*Test,Test*`.
   
   I think this should be changed to use the JUnit Assumption instead.



-- 
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: notifications-unsubscribe@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org