You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/10/07 16:28:47 UTC

[GitHub] [geode] jdeppe-pivotal opened a new pull request #5601: Expand sub classes indicated by StressNewTest

jdeppe-pivotal opened a new pull request #5601:
URL: https://github.com/apache/geode/pull/5601


   Thank you for submitting a contribution to Apache Geode.
   
   In order to streamline the review of the contribution we ask you
   to ensure the following steps have been taken:
   
   ### For all changes:
   - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
   
   - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
   
   - [ ] Is your initial contribution a single, squashed commit?
   
   - [ ] Does `gradlew build` run cleanly?
   
   - [ ] Have you written or updated unit tests to verify your changes?
   
   - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
   
   ### Note:
   Please ensure that once the PR is submitted, check Concourse for build issues and
   submit an update to your PR as soon as possible. If you need help, please send an
   email to dev@geode.apache.org.
   


----------------------------------------------------------------
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] [geode] rhoughton-pivot commented on pull request #5601: GEODE-8603: Potentially expand classes identified for CI stressing to include subclasses

Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on pull request #5601:
URL: https://github.com/apache/geode/pull/5601#issuecomment-714704994


   I wish that the creation of what to run was encapsulated entirely in build with less `bash` needed, but this seems like a good step in the right direction. Eventually, I think the StressNewTestHelper can move to `buildSrc` as a plugin, feeding to the `repeatTest` task for auto-determining what to run.
   


----------------------------------------------------------------
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] [geode] upthewaterspout commented on a change in pull request #5601: WIP: Expand sub classes indicated by StressNewTest

Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on a change in pull request #5601:
URL: https://github.com/apache/geode/pull/5601#discussion_r501982046



##########
File path: geode-junit/src/main/java/org/apache/geode/test/util/ClassScanner.java
##########
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.test.util;
+
+import java.io.File;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import io.github.classgraph.ClassGraph;
+import io.github.classgraph.ClassInfo;
+import io.github.classgraph.ScanResult;
+
+/**
+ * This class is intended as a helper to the CI StressNewTest job.
+ */
+public class ClassScanner {
+
+  private ScanResult scanResult;
+  private String packageToScan;
+
+  public ClassScanner(String packageToScan) {
+    this.packageToScan = packageToScan;
+    scanResult = new ClassGraph().whitelistPackages(packageToScan)
+        .enableClassInfo()
+        .enableAnnotationInfo().scan();
+  }
+
+  public List<String> whatExtends(String classOrFile) {
+    Set<String> classesToConsider = new HashSet<>();
+    classesToConsider.add(pathToClass(classOrFile));
+
+    if (!classOrFile.contains(".")) {
+      classesToConsider.addAll(fullyQualifyClass(classOrFile));
+    }
+
+    return classesToConsider.stream()
+        .flatMap(x -> scanResult.getSubclasses(x).stream().map(ClassInfo::getSimpleName))

Review comment:
       Is this possibly missing running concrete classes that also have subclasses? Like someone extended a real test with more subclasses?




----------------------------------------------------------------
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] [geode] jdeppe-pivotal commented on a change in pull request #5601: WIP: Expand sub classes indicated by StressNewTest

Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5601:
URL: https://github.com/apache/geode/pull/5601#discussion_r503456660



##########
File path: geode-junit/src/main/java/org/apache/geode/test/util/ClassScanner.java
##########
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.test.util;
+
+import java.io.File;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import io.github.classgraph.ClassGraph;
+import io.github.classgraph.ClassInfo;
+import io.github.classgraph.ScanResult;
+
+/**
+ * This class is intended as a helper to the CI StressNewTest job.
+ */
+public class ClassScanner {
+
+  private ScanResult scanResult;
+  private String packageToScan;
+
+  public ClassScanner(String packageToScan) {
+    this.packageToScan = packageToScan;
+    scanResult = new ClassGraph().whitelistPackages(packageToScan)
+        .enableClassInfo()
+        .enableAnnotationInfo().scan();
+  }
+
+  public List<String> whatExtends(String classOrFile) {
+    Set<String> classesToConsider = new HashSet<>();
+    classesToConsider.add(pathToClass(classOrFile));
+
+    if (!classOrFile.contains(".")) {
+      classesToConsider.addAll(fullyQualifyClass(classOrFile));
+    }
+
+    return classesToConsider.stream()
+        .flatMap(x -> scanResult.getSubclasses(x).stream().map(ClassInfo::getSimpleName))

Review comment:
       In that case the selected subclass would end up executing tests in the super. A test could be missed if the subclass overrides a test in the super. 




----------------------------------------------------------------
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] [geode] upthewaterspout commented on a change in pull request #5601: WIP: Expand sub classes indicated by StressNewTest

Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on a change in pull request #5601:
URL: https://github.com/apache/geode/pull/5601#discussion_r501982046



##########
File path: geode-junit/src/main/java/org/apache/geode/test/util/ClassScanner.java
##########
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.test.util;
+
+import java.io.File;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import io.github.classgraph.ClassGraph;
+import io.github.classgraph.ClassInfo;
+import io.github.classgraph.ScanResult;
+
+/**
+ * This class is intended as a helper to the CI StressNewTest job.
+ */
+public class ClassScanner {
+
+  private ScanResult scanResult;
+  private String packageToScan;
+
+  public ClassScanner(String packageToScan) {
+    this.packageToScan = packageToScan;
+    scanResult = new ClassGraph().whitelistPackages(packageToScan)
+        .enableClassInfo()
+        .enableAnnotationInfo().scan();
+  }
+
+  public List<String> whatExtends(String classOrFile) {
+    Set<String> classesToConsider = new HashSet<>();
+    classesToConsider.add(pathToClass(classOrFile));
+
+    if (!classOrFile.contains(".")) {
+      classesToConsider.addAll(fullyQualifyClass(classOrFile));
+    }
+
+    return classesToConsider.stream()
+        .flatMap(x -> scanResult.getSubclasses(x).stream().map(ClassInfo::getSimpleName))

Review comment:
       Is this possibly missing running concrete classes that also have subclasses? Like someone extended a real test with more subclasses?




----------------------------------------------------------------
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] [geode] onichols-pivotal commented on a change in pull request #5601: GEODE-8603: Potentially expand classes identified for CI stressing to include subclasses

Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on a change in pull request #5601:
URL: https://github.com/apache/geode/pull/5601#discussion_r510442090



##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/AbstractPubSubIntegrationTest.java
##########
@@ -51,6 +51,7 @@
 import org.apache.geode.test.junit.rules.ExecutorServiceRule;
 
 public abstract class AbstractPubSubIntegrationTest implements RedisPortSupplier {
+  /* Trigger a stress test run */

Review comment:
       was this intended to be left in?




----------------------------------------------------------------
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] [geode] jdeppe-pivotal commented on a change in pull request #5601: GEODE-8603: Potentially expand classes identified for CI stressing to include subclasses

Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5601:
URL: https://github.com/apache/geode/pull/5601#discussion_r510445333



##########
File path: geode-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/pubsub/AbstractPubSubIntegrationTest.java
##########
@@ -51,6 +51,7 @@
 import org.apache.geode.test.junit.rules.ExecutorServiceRule;
 
 public abstract class AbstractPubSubIntegrationTest implements RedisPortSupplier {
+  /* Trigger a stress test run */

Review comment:
       Thanks for pointing that out - I'll remove it.




----------------------------------------------------------------
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] [geode] rhoughton-pivot commented on a change in pull request #5601: WIP: Expand sub classes indicated by StressNewTest

Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on a change in pull request #5601:
URL: https://github.com/apache/geode/pull/5601#discussion_r502023031



##########
File path: ci/scripts/repeat-new-tests.sh
##########
@@ -46,26 +46,56 @@ function changes_for_path() {
   popd >> /dev/null
 }
 
-UNIT_TEST_CHANGES=$(changes_for_path '*/src/test/java') || exit $?
-INTEGRATION_TEST_CHANGES=$(changes_for_path '*/src/integrationTest/java') || exit $?
-DISTRIBUTED_TEST_CHANGES=$(changes_for_path '*/src/distributedTest/java') || exit $?
-ACCEPTANCE_TEST_CHANGES=$(changes_for_path '*/src/acceptanceTest/java') || exit $?
-UPGRADE_TEST_CHANGES=$(changes_for_path '*/src/upgradeTest/java') || exit $?
+function save_classpath() {
+  echo "Building and saving classpath"
+  pushd geode >> /dev/null
+    # Do this twice since devBuild still dumps a warning string to stdout.
+    ./gradlew --console=plain -q devBuild 2>/dev/null
+    ./gradlew --console=plain -q printTestClasspath 2>/dev/null >/tmp/classpath.txt

Review comment:
       What is the behavior if devBuild has not been run first?




----------------------------------------------------------------
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] [geode] rhoughton-pivot commented on a change in pull request #5601: WIP: Expand sub classes indicated by StressNewTest

Posted by GitBox <gi...@apache.org>.
rhoughton-pivot commented on a change in pull request #5601:
URL: https://github.com/apache/geode/pull/5601#discussion_r502023031



##########
File path: ci/scripts/repeat-new-tests.sh
##########
@@ -46,26 +46,56 @@ function changes_for_path() {
   popd >> /dev/null
 }
 
-UNIT_TEST_CHANGES=$(changes_for_path '*/src/test/java') || exit $?
-INTEGRATION_TEST_CHANGES=$(changes_for_path '*/src/integrationTest/java') || exit $?
-DISTRIBUTED_TEST_CHANGES=$(changes_for_path '*/src/distributedTest/java') || exit $?
-ACCEPTANCE_TEST_CHANGES=$(changes_for_path '*/src/acceptanceTest/java') || exit $?
-UPGRADE_TEST_CHANGES=$(changes_for_path '*/src/upgradeTest/java') || exit $?
+function save_classpath() {
+  echo "Building and saving classpath"
+  pushd geode >> /dev/null
+    # Do this twice since devBuild still dumps a warning string to stdout.
+    ./gradlew --console=plain -q devBuild 2>/dev/null
+    ./gradlew --console=plain -q printTestClasspath 2>/dev/null >/tmp/classpath.txt

Review comment:
       What is the behavior if devBuild has not been run first?




----------------------------------------------------------------
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] [geode] jdeppe-pivotal commented on a change in pull request #5601: WIP: Expand sub classes indicated by StressNewTest

Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5601:
URL: https://github.com/apache/geode/pull/5601#discussion_r503457325



##########
File path: ci/scripts/repeat-new-tests.sh
##########
@@ -46,26 +46,56 @@ function changes_for_path() {
   popd >> /dev/null
 }
 
-UNIT_TEST_CHANGES=$(changes_for_path '*/src/test/java') || exit $?
-INTEGRATION_TEST_CHANGES=$(changes_for_path '*/src/integrationTest/java') || exit $?
-DISTRIBUTED_TEST_CHANGES=$(changes_for_path '*/src/distributedTest/java') || exit $?
-ACCEPTANCE_TEST_CHANGES=$(changes_for_path '*/src/acceptanceTest/java') || exit $?
-UPGRADE_TEST_CHANGES=$(changes_for_path '*/src/upgradeTest/java') || exit $?
+function save_classpath() {
+  echo "Building and saving classpath"
+  pushd geode >> /dev/null
+    # Do this twice since devBuild still dumps a warning string to stdout.
+    ./gradlew --console=plain -q devBuild 2>/dev/null
+    ./gradlew --console=plain -q printTestClasspath 2>/dev/null >/tmp/classpath.txt

Review comment:
       No code is compiled which is needed for the subsequent `StressNewTestHelper` to be executed.




----------------------------------------------------------------
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] [geode] jdeppe-pivotal commented on pull request #5601: WIP: Expand sub classes indicated by StressNewTest

Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on pull request #5601:
URL: https://github.com/apache/geode/pull/5601#issuecomment-707273038


   After some rework I ended up having the java code take on building the full gradle test task command line. This is because a subclass does not necessarily exist within the same source set than its superclass. This situation then requires executing the subclass under a different gradle task.


----------------------------------------------------------------
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] [geode] jdeppe-pivotal merged pull request #5601: GEODE-8603: Potentially expand classes identified for CI stressing to include subclasses

Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal merged pull request #5601:
URL: https://github.com/apache/geode/pull/5601


   


----------------------------------------------------------------
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