xintongsong commented on a change in pull request #15279:
URL: https://github.com/apache/flink/pull/15279#discussion_r598382161
##########
File path:
flink-core/src/test/java/org/apache/flink/testutils/ThrowOnDoubleMemoryFreeing.java
##########
@@ -19,32 +19,28 @@
package org.apache.flink.testutils;
import org.apache.flink.core.memory.MemorySegment;
-import org.apache.flink.core.testutils.CommonTestUtils;
import org.apache.flink.util.ExternalResource;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
+import java.util.Properties;
/**
* A rule that can be used to enable throwing exception on multiple freeing of
{@link
* MemorySegment}.
*/
public class ThrowOnDoubleMemoryFreeing implements ExternalResource {
- private Map<String, String> systemEnv = new HashMap<>();
+ private Properties systemProperties;
@Override
public void before() throws Exception {
- systemEnv = System.getenv();
- CommonTestUtils.setEnv(
-
Collections.singletonMap(MemorySegment.CHECK_MULTIPLE_FREE_PROPERTY, ""),
false);
+ systemProperties = System.getProperties();
+ System.setProperty(MemorySegment.CHECK_MULTIPLE_FREE_PROPERTY, "");
Review comment:
Double-checking the maven surefire plugin documents, I think you are
right. No isolation is guaranteed between unit tests.
This approach came from this [PR
discussion](https://github.com/apache/flink/pull/15246#discussion_r595776849).
We are not ready to fail on double memory freeing for all the tests yet. And if
it doesn't fail, the test cases added in the PR does not protect anything.
Maybe we can remove `ThrowOnDoubleMemoryFreeing`, and leave the test cases
to only take effect when the failings are activated. We may use `Assume` to
ignore the cases when the property if not set.
cc @dawidwys
--
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:
[email protected]
