[GitHub] [flink] azagrebin commented on a change in pull request #9258: [FLINK-13460][tests] Rework SerializedThrowableTest to not use Unsafe#defineClass()
azagrebin commented on a change in pull request #9258: [FLINK-13460][tests]
Rework SerializedThrowableTest to not use Unsafe#defineClass()
URL: https://github.com/apache/flink/pull/9258#discussion_r314340625
##
File path:
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/core/testutils/CommonTestUtils.java
##
@@ -159,14 +159,14 @@ public static void setEnv(Map newenv,
boolean clearExisting) {
/**
* A new object and the corresponding ClassLoader for that object, as
returned by
-* {@link #createObjectFromNewClassLoader(ClassLoader)}.
+* {@link #createSerializableObjectFromNewClassLoader(ClassLoader)}.
Review comment:
It is only about jdoc for `ObjectAndClassLoader`.
`ObjectAndClassLoader` is returned from all methods:
`createSerializableObjectFromNewClassLoader`,
`createExceptionObjectFromNewClassLoader` and still
`createObjectFromNewClassLoader` (now private).
`T extends Serializable` is just a thought, maybe not needed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] azagrebin commented on a change in pull request #9258: [FLINK-13460][tests] Rework SerializedThrowableTest to not use Unsafe#defineClass()
azagrebin commented on a change in pull request #9258: [FLINK-13460][tests]
Rework SerializedThrowableTest to not use Unsafe#defineClass()
URL: https://github.com/apache/flink/pull/9258#discussion_r314248198
##
File path:
flink-test-utils-parent/flink-test-utils-junit/src/test/java/org/apache/flink/core/testutils/CommonTestUtilsTest.java
##
@@ -45,7 +47,29 @@ public void testObjectFromNewClassLoaderObject() throws
Exception {
@Test
public void testObjectFromNewClassLoaderClassLoaders() throws Exception
{
- final CommonTestUtils.ObjectAndClassLoader objectAndClassLoader
= CommonTestUtils.createObjectFromNewClassLoader();
+ final CommonTestUtils.ObjectAndClassLoader
objectAndClassLoader =
CommonTestUtils.createSerializableObjectFromNewClassLoader();
+
+ assertNotEquals(ClassLoader.getSystemClassLoader(),
objectAndClassLoader.getClassLoader());
+ assertEquals(ClassLoader.getSystemClassLoader(),
objectAndClassLoader.getClassLoader().getParent());
+ }
+
+ @Test
+ public void testExceptionObjectFromNewClassLoaderObject() throws
Exception {
+ final CommonTestUtils.ObjectAndClassLoader
objectAndClassLoader =
CommonTestUtils.createExceptionObjectFromNewClassLoader();
+ final Object o = objectAndClassLoader.getObject();
+
+ assertNotEquals(ClassLoader.getSystemClassLoader(),
o.getClass().getClassLoader());
+
+ try {
+ Class.forName(o.getClass().getName());
+ fail("should not be able to load class from the system
class loader");
Review comment:
ok, I see, it is actually about object and class loader test separation.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] azagrebin commented on a change in pull request #9258: [FLINK-13460][tests] Rework SerializedThrowableTest to not use Unsafe#defineClass()
azagrebin commented on a change in pull request #9258: [FLINK-13460][tests]
Rework SerializedThrowableTest to not use Unsafe#defineClass()
URL: https://github.com/apache/flink/pull/9258#discussion_r314246617
##
File path:
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/core/testutils/CommonTestUtils.java
##
@@ -159,14 +159,14 @@ public static void setEnv(Map newenv,
boolean clearExisting) {
/**
* A new object and the corresponding ClassLoader for that object, as
returned by
-* {@link #createObjectFromNewClassLoader(ClassLoader)}.
+* {@link #createSerializableObjectFromNewClassLoader(ClassLoader)}.
Review comment:
I did not mean this particular method name
`createSerializableObjectFromNewClassLoader`.
`ObjectAndClassLoader` is also returned by the exception version of
`createObjectFromNewClassLoader`.
Ok, it is nit anyways: could add `as E.G. as returned by`
If `Serializab`ility is important, it could be also bounded type parameter:
`T extends Serializable`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] azagrebin commented on a change in pull request #9258: [FLINK-13460][tests] Rework SerializedThrowableTest to not use Unsafe#defineClass()
azagrebin commented on a change in pull request #9258: [FLINK-13460][tests]
Rework SerializedThrowableTest to not use Unsafe#defineClass()
URL: https://github.com/apache/flink/pull/9258#discussion_r313904396
##
File path:
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/core/testutils/CommonTestUtils.java
##
@@ -159,14 +159,14 @@ public static void setEnv(Map newenv,
boolean clearExisting) {
/**
* A new object and the corresponding ClassLoader for that object, as
returned by
-* {@link #createObjectFromNewClassLoader(ClassLoader)}.
+* {@link #createSerializableObjectFromNewClassLoader(ClassLoader)}.
Review comment:
I guess **Serializable** should be removed from
`createSerializableObjectFromNewClassLoader` as it is not strongly relevant
anymore for `ObjectAndClassLoader`.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] azagrebin commented on a change in pull request #9258: [FLINK-13460][tests] Rework SerializedThrowableTest to not use Unsafe#defineClass()
azagrebin commented on a change in pull request #9258: [FLINK-13460][tests]
Rework SerializedThrowableTest to not use Unsafe#defineClass()
URL: https://github.com/apache/flink/pull/9258#discussion_r313927314
##
File path:
flink-test-utils-parent/flink-test-utils-junit/src/test/java/org/apache/flink/core/testutils/CommonTestUtilsTest.java
##
@@ -45,7 +47,29 @@ public void testObjectFromNewClassLoaderObject() throws
Exception {
@Test
public void testObjectFromNewClassLoaderClassLoaders() throws Exception
{
- final CommonTestUtils.ObjectAndClassLoader objectAndClassLoader
= CommonTestUtils.createObjectFromNewClassLoader();
+ final CommonTestUtils.ObjectAndClassLoader
objectAndClassLoader =
CommonTestUtils.createSerializableObjectFromNewClassLoader();
Review comment:
maybe factor out one common helper test method where it does not matter
whether it is `Serializable` or `Exception`?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] azagrebin commented on a change in pull request #9258: [FLINK-13460][tests] Rework SerializedThrowableTest to not use Unsafe#defineClass()
azagrebin commented on a change in pull request #9258: [FLINK-13460][tests]
Rework SerializedThrowableTest to not use Unsafe#defineClass()
URL: https://github.com/apache/flink/pull/9258#discussion_r313906377
##
File path:
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/core/testutils/CommonTestUtils.java
##
@@ -185,26 +185,152 @@ public Serializable getObject() {
* This is useful when unit testing the class loading behavior of code,
and needing a class that
* is outside the system class path.
*
-* This method behaves like {@code
createObjectFromNewClassLoader(ClassLoader.getSystemClassLoader())}.
+* This method behaves like {@code
createSerializableObjectFromNewClassLoader(ClassLoader.getSystemClassLoader())}.
*/
- public static ObjectAndClassLoader createObjectFromNewClassLoader() {
- return
createObjectFromNewClassLoader(ClassLoader.getSystemClassLoader());
+ public static ObjectAndClassLoader
createSerializableObjectFromNewClassLoader() {
+ return
createSerializableObjectFromNewClassLoader(ClassLoader.getSystemClassLoader());
}
/**
* Creates a new ClassLoader and a new class inside that ClassLoader.
* This is useful when unit testing the class loading behavior of code,
and needing a class that
* is outside the system class path.
*
+* This method behaves like {@code
createExceptionObjectFromNewClassLoader(ClassLoader.getSystemClassLoader())}.
+*/
+ public static ObjectAndClassLoader
createExceptionObjectFromNewClassLoader() {
+ return
createExceptionObjectFromNewClassLoader(ClassLoader.getSystemClassLoader());
+ }
+
+ /**
+* Creates a new ClassLoader and a new {@link Serializable} class
inside that ClassLoader.
+* This is useful when unit testing the class loading behavior of code,
and needing a class that
+* is outside the system class path.
+*
* NOTE: Even though this method may throw IOExceptions, we do not
declare those and rather
* wrap them in Runtime Exceptions. While this is generally
discouraged, we do this here because it
* is merely a test utility and not production code, and it makes it
easier to use this method
* during the initialization of variables and especially static
variables.
*
* @param parentClassLoader The parent class loader used for the newly
created class loader.
*/
- public static ObjectAndClassLoader
createObjectFromNewClassLoader(@Nullable ClassLoader parentClassLoader) {
- final String testClassName =
"org.apache.flink.TestSerializable";
+ public static ObjectAndClassLoader
createSerializableObjectFromNewClassLoader(@Nullable ClassLoader
parentClassLoader) {
+ // this is the byte code of the following simple class:
+ //
--
+ // package org.apache.flink;
+ //
+ // public class TestSerializable implements
java.io.Serializable {}
+ //
--
+
+ final byte[] classData = {-54, -2, -70, -66, 0, 0, 0, 51, 0,
65, 10, 0, 15, 0, 43, 7, 0, 44,
+ 10, 0, 2, 0, 43, 10, 0, 2, 0, 45, 9, 0, 7, 0, 46, 10,
0, 15, 0, 47, 7, 0, 48, 7, 0,
+ 49, 10, 0, 8, 0, 43, 8, 0, 50, 10, 0, 8, 0, 51, 10, 0,
8, 0, 52, 10, 0, 8, 0, 53, 10,
+ 0, 8, 0, 54, 7, 0, 55, 7, 0, 56, 1, 0, 16, 115, 101,
114, 105, 97, 108, 86, 101, 114,
+ 115, 105, 111, 110, 85, 73, 68, 1, 0, 1, 74, 1, 0, 13,
67, 111, 110, 115, 116, 97, 110,
+ 116, 86, 97, 108, 117, 101, 5, -1, -1, -1, -1, -1, -1,
-1, -3, 1, 0, 6, 114, 97, 110,
+ 100, 111, 109, 1, 0, 6, 60, 105, 110, 105, 116, 62, 1,
0, 3, 40, 41, 86, 1, 0, 4, 67,
+ 111, 100, 101, 1, 0, 15, 76, 105, 110, 101, 78, 117,
109, 98, 101, 114, 84, 97, 98, 108,
+ 101, 1, 0, 18, 76, 111, 99, 97, 108, 86, 97, 114, 105,
97, 98, 108, 101, 84, 97, 98,
+ 108, 101, 1, 0, 4, 116, 104, 105, 115, 1, 0, 35, 76,
111, 114, 103, 47, 97, 112, 97, 99,
+ 104, 101, 47, 102, 108, 105, 110, 107, 47, 84, 101,
115, 116, 83, 101, 114, 105, 97, 108,
+ 105, 122, 97, 98, 108, 101, 59, 1, 0, 6, 101, 113, 117,
97, 108, 115, 1, 0, 21, 40, 76,
+ 106, 97, 118, 97, 47, 108, 97, 110, 103, 47, 79, 98,
106, 101, 99, 116, 59, 41, 90, 1, 0,
+ 1, 111, 1, 0, 18, 76, 106, 97, 118, 97, 47, 108, 97,
110, 103, 47, 79, 98, 106, 101, 99,
+ 116, 59, 1, 0, 4, 116, 104, 97, 116, 1, 0, 13, 83, 116,
97, 99, 107,
[GitHub] [flink] azagrebin commented on a change in pull request #9258: [FLINK-13460][tests] Rework SerializedThrowableTest to not use Unsafe#defineClass()
azagrebin commented on a change in pull request #9258: [FLINK-13460][tests]
Rework SerializedThrowableTest to not use Unsafe#defineClass()
URL: https://github.com/apache/flink/pull/9258#discussion_r313925953
##
File path:
flink-test-utils-parent/flink-test-utils-junit/src/test/java/org/apache/flink/core/testutils/CommonTestUtilsTest.java
##
@@ -45,7 +47,29 @@ public void testObjectFromNewClassLoaderObject() throws
Exception {
@Test
public void testObjectFromNewClassLoaderClassLoaders() throws Exception
{
- final CommonTestUtils.ObjectAndClassLoader objectAndClassLoader
= CommonTestUtils.createObjectFromNewClassLoader();
+ final CommonTestUtils.ObjectAndClassLoader
objectAndClassLoader =
CommonTestUtils.createSerializableObjectFromNewClassLoader();
+
+ assertNotEquals(ClassLoader.getSystemClassLoader(),
objectAndClassLoader.getClassLoader());
+ assertEquals(ClassLoader.getSystemClassLoader(),
objectAndClassLoader.getClassLoader().getParent());
+ }
+
+ @Test
+ public void testExceptionObjectFromNewClassLoaderObject() throws
Exception {
+ final CommonTestUtils.ObjectAndClassLoader
objectAndClassLoader =
CommonTestUtils.createExceptionObjectFromNewClassLoader();
+ final Object o = objectAndClassLoader.getObject();
+
+ assertNotEquals(ClassLoader.getSystemClassLoader(),
o.getClass().getClassLoader());
+
+ try {
+ Class.forName(o.getClass().getName());
+ fail("should not be able to load class from the system
class loader");
Review comment:
could we just have this check in other tests/one helper test method?
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:
us...@infra.apache.org
With regards,
Apache Git Services
