[GitHub] [dolphinscheduler] kezhenxu94 commented on a change in pull request #9077: [Feature][e2e] Suggest e2e test adapt M1 chip

[GitBox <gi...@apache.org>]

kezhenxu94 commented on a change in pull request #9077:
URL: https://github.com/apache/dolphinscheduler/pull/9077#discussion_r831785408



##########
File path: dolphinscheduler-e2e/dolphinscheduler-e2e-core/src/main/java/org/apache/dolphinscheduler/e2e/core/DolphinSchedulerExtension.java
##########
@@ -59,12 +59,13 @@
 import com.google.common.net.HostAndPort;
 
 import lombok.extern.slf4j.Slf4j;
+import org.testcontainers.utility.DockerImageName;
 
 @Slf4j
-final class DolphinSchedulerExtension
-    implements BeforeAllCallback, AfterAllCallback,
-    BeforeEachCallback {
-    private final boolean LOCAL_MODE = Objects.equals(System.getProperty("local"), "true");
+final class DolphinSchedulerExtension implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback {

Review comment:
       Please try to avoid these unnecessary changes

##########
File path: dolphinscheduler-e2e/dolphinscheduler-e2e-core/src/main/java/org/apache/dolphinscheduler/e2e/core/DolphinSchedulerExtension.java
##########
@@ -159,6 +151,39 @@ public Statement apply(Statement base, Description description) {
         rootPath = "/dolphinscheduler/ui/";
     }
 
+    private void getBrowserContainerByOsName() {
+        if (LOCAL_MODE && M1_CHIP_FLAG) {
+            imageName = DockerImageName.parse("seleniarm/standalone-chromium:4.1.2-20220227")

Review comment:
       If this image also works for non-M1, I'd just upgrade the image for all systems, not just M1

##########
File path: dolphinscheduler-e2e/dolphinscheduler-e2e-core/src/main/java/org/apache/dolphinscheduler/e2e/core/DolphinSchedulerExtension.java
##########
@@ -73,36 +74,27 @@
     private HostAndPort address;
     private String rootPath;
 
+    private DockerImageName imageName;
+
+    private Path record;
+
+
     @Override
     @SuppressWarnings("UnstableApiUsage")
     public void beforeAll(ExtensionContext context) throws IOException {
         Awaitility.setDefaultTimeout(Duration.ofSeconds(60));
         Awaitility.setDefaultPollInterval(Duration.ofSeconds(10));
 
+        getRecordPath();
+
         if (LOCAL_MODE) {
             runInLocal();
         } else {
             runInDockerContainer(context);
         }
 
-        final Path record;
-        if (!Strings.isNullOrEmpty(System.getenv("RECORDING_PATH"))) {
-            record = Paths.get(System.getenv("RECORDING_PATH"));
-            if (!record.toFile().exists()) {
-                if (!record.toFile().mkdir()) {
-                    throw new IOException("Failed to create recording directory: " + record.toAbsolutePath());
-                }
-            }
-        } else {
-            record = Files.createTempDirectory("record-");
-        }
+        getBrowserContainerByOsName();

Review comment:
       `getXxxx` usually returns an object and is assigned to some variable, but your  `getXxxx` instead, sets a field, which I don't think is a good practice.
   
   Please either change the method name to `setXxx` or make it return the object and assign it to the field

##########
File path: dolphinscheduler-e2e/dolphinscheduler-e2e-core/src/main/java/org/apache/dolphinscheduler/e2e/core/DolphinSchedulerExtension.java
##########
@@ -73,36 +74,27 @@
     private HostAndPort address;
     private String rootPath;
 
+    private DockerImageName imageName;

Review comment:
       This can be a local variable, it doesn't need to be a field

##########
File path: dolphinscheduler-e2e/dolphinscheduler-e2e-core/src/main/java/org/apache/dolphinscheduler/e2e/core/DolphinSchedulerExtension.java
##########
@@ -59,12 +59,13 @@
 import com.google.common.net.HostAndPort;
 
 import lombok.extern.slf4j.Slf4j;
+import org.testcontainers.utility.DockerImageName;
 
 @Slf4j
-final class DolphinSchedulerExtension
-    implements BeforeAllCallback, AfterAllCallback,
-    BeforeEachCallback {
-    private final boolean LOCAL_MODE = Objects.equals(System.getProperty("local"), "true");
+final class DolphinSchedulerExtension implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback {
+    private final boolean LOCAL_MODE = Objects.equals(System.getenv("local"), "true");
+
+    private final boolean M1_CHIP_FLAG = Objects.equals(System.getenv("m1_chip"), "true");

Review comment:
       We are using system properties, not environment variables




-- 
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: commits-unsubscribe@dolphinscheduler.apache.org

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