[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #560: [#559] refactor: use system stubs to mutable environment variables

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

advancedxy commented on code in PR #560:
URL: https://github.com/apache/incubator-uniffle/pull/560#discussion_r1098839195


##########
common/src/test/java/org/apache/uniffle/common/util/RssUtilsTest.java:
##########
@@ -43,7 +46,16 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
-public class RssUtilsTest {
+@ExtendWith(SystemStubsExtension.class)
+public class RssUtilsTest implements EnvironmentVariablesSetter {

Review Comment:
   `EnvTestBase` is defined in storage package, so it cannot be extended by `RssUtilsTest`.
   
   Why `EnvTestBase` is defined in storage rather than common package? 
   Because maven doesn't support transitive  test dependency[^1], which means if we put `EnvTestBase` in common, every other modules' pom.xml would redeclare the test-jar dependency, which is tedious. Something like below:
   
   ```
       <dependency>
         <groupId>org.apache.uniffle</groupId>
         <artifactId>rss-common</artifactId>
         <type>test-jar</type>
         <scope>test</scope>
       </dependency>
   ```
   
   [^1]: https://issues.apache.org/jira/browse/MNG-1378



-- 
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@uniffle.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org