Tests for isAccessUserOnly0 native #1656

theresa-m · 2018-04-11T00:08:12Z

includes tests for isAccessUserOnly0 in Java8 and Java9
expanded test infra to have src_90_current and src_90_latest files for different JCL versions in Java 9

Relies on: #1655

Signed-off-by: Theresa Mammarella Theresa.T.Mammarella@ibm.com

theresa-m · 2018-04-11T00:10:24Z

test/Java8andUp/src_80/org/openj9/test/jdk/internal/agent/Test_FileSystem.java

+		}
+
+		FileSystem fs = FileSystem.open();
+		File file = new File("test-file-name");


@smlambert concerns with this file created for testing? I've made sure to delete it at the end of each test.

Creating a file is not an issue (as you clean it up after). There are some other test changes that we will ask for on this PR, I've added @llxia for a review.

smlambert · 2018-04-11T01:17:00Z

test/Java8andUp/playlist.xml

@@ -1843,5 +1843,46 @@
 			<subset>SE90</subset>
 		</subsets>
 	</test>
+
+	<test>
+		<testCaseName>FileSystem-isAccessUserOnlyTests_SE90</testCaseName>


For tests that should apply to Java9 and Up, its preferred to call the Java8 tests _SE80, and then drop the suffix, so the same test can be used for 9, 10, 11 and so on.

smlambert · 2018-04-11T01:20:19Z

test/Java8andUp/src_80/org/openj9/test/jdk/internal/agent/Test_FileSystem.java

+ *******************************************************************************/
+
+import org.testng.annotations.Test;
+import org.testng.AssertJUnit;


For brand new test code, you could just import org.testng.Assert; and we should update the example, so that it uses it to. Some tests use AssertJUnit because they were converted using a JUnit-to-TestNG plugin, and the ordering of the arguments for JUnit asserts are reversed from the order of TestNG asserts.

smlambert · 2018-04-11T01:26:52Z

I have not done a diff between your Test_FileSystem.java code for src_80, src_90_latest, and src_90_current, but seems like a good opportunity to reuse some code (see examples of reuse).

Another question, perhaps for @DanHeidinga, is it necessary to add tests for src_90_current, does this refer to b148 class libraries, and when can we stop adding tests to continue to support that JCL version)?

DanHeidinga · 2018-04-11T01:58:06Z

@theresa-m Have you seen some of the createTempFile api calls like: https://docs.oracle.com/javase/9/docs/api/java/io/File.html#createTempFile-java.lang.String-java.lang.String-java.io.File-

Using those calls avoids having to name the file and ensures that the tests can be run in parallel as the file(s) will be unique.

The code explicitly deleting the file is good and to cover all the cases, please also call https://docs.oracle.com/javase/9/docs/api/java/io/File.html#deleteOnExit-- on the files.

DanHeidinga · 2018-04-11T02:07:11Z

Another question, perhaps for @DanHeidinga, is it necessary to add tests for src_90_current, does this refer to b148 class libraries, and when can we stop adding tests to continue to support that JCL version)?

We should not be adding new tests to support b148. That's an IBM-ism

llxia · 2018-04-11T13:17:47Z

test/Java8andUp/playlist.xml

+			<group>functional</group>
+		</groups>
+		<subsets>
+			<subset>SE90</subset>


also, if the test applies to Java9 and Up, then please add subset

<subset>SE100</subset> <subset>SE110</subset>

If you debase your branch, you will see tests in this playlist.xml is updated to have SE100 and SE110.

theresa-m · 2018-04-11T14:10:42Z

Yes reusing code for this will be great. The files are exactly the same except for the package name of the class they are testing. Thanks for the link @smlambert

theresa-m · 2018-04-11T14:12:45Z

The addition of the current/latest folders for Java 9 was mainly because of internal test compile failures. The FileSystem class being tested changed locations between jcl versions so my test would not work on both jcls. Are there some better ways this could work?

theresa-m · 2018-04-13T03:18:49Z

Ready for another review.

llxia · 2018-04-13T13:13:52Z

test/Java8andUp/build.xml

+							</classpath>
+						</javac>
+					</else>
+				</if>


Instead of using if else, could we use variables to avoid code duplication? The differences between if and else are the following:

${src_90_current} and ${src_90_latest}

--add-exports java.management/sun.management=ALL-UNNAMED and --add-exports jdk.management.agent/jdk.internal.agent=ALL-UNNAMED

good idea. I just pushed this update

llxia · 2018-04-13T18:55:11Z

Jenkins test sanity plinux jdk9,jdk10

theresa-m · 2018-04-13T19:14:06Z

Marking as WIP so this does not get merged before #1655

llxia · 2018-04-13T19:18:04Z

Aborted both test builds as they will not pass without #1655

theresa-m · 2018-05-17T22:51:00Z

@llxia the dependencies for this pr have been merged

theresa-m · 2018-05-17T23:01:34Z

rebasing as this is a while behind.

theresa-m · 2018-05-18T01:00:17Z

Jenkins test sanity plinux jdk9,jdk10

edit: looks like I don't have permission to do this :)

pshipton · 2018-05-18T01:04:08Z

Jenkins test sanity plinux jdk9,jdk10

pshipton · 2018-05-18T01:09:16Z

Jenkins test sanity plinux jdk8

theresa-m · 2018-05-18T18:00:10Z

Jdk10 test failure is fixed pending #1942

pshipton · 2018-05-19T00:22:42Z

jenkins test sanity plinux jdk10

pshipton · 2018-05-19T04:42:57Z

@llxia @smlambert ready for another look

llxia · 2018-05-22T14:58:14Z

test/functional/TestUtilities/src/org/openj9/test/jdk/internal/agent/Test_FileSystem_Base.java

+
+	protected void resolveTest(boolean actualResult, boolean expectedResult) {
+		logger.debug("Test passed: " + (expectedResult == actualResult));
+		Assert.assertEquals(expectedResult, actualResult);


Do we need logger.debug("Test passed: " + (expectedResult == actualResult))? Does Assert.assertEquals(expectedResult, actualResult) provide the same information?

its not very helpful. I will take that out.

llxia

the rest looks good to me.

- includes tests for isAccessUserOnly0 in Java8 and Java9 - expanded test infra to have src_90_current and src_90_latest files for different JCL versions in Java 9 Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>

Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>

theresa-m · 2018-05-22T15:14:02Z

updated with Lan's latest comment

theresa-m · 2018-05-23T12:45:26Z

@pshipton this is ready to go

theresa-m mentioned this pull request Apr 11, 2018

Add isAccessUserOnly0 for FileSystem #1655

Merged

theresa-m commented Apr 11, 2018

View reviewed changes

theresa-m force-pushed the fix_1520_tests branch from c2fe526 to da3f76c Compare April 11, 2018 00:12

smlambert added the comp:test label Apr 11, 2018

smlambert requested a review from llxia April 11, 2018 01:13

smlambert reviewed Apr 11, 2018

View reviewed changes

llxia reviewed Apr 11, 2018

View reviewed changes

theresa-m force-pushed the fix_1520_tests branch from da3f76c to bb11904 Compare April 13, 2018 03:18

llxia reviewed Apr 13, 2018

View reviewed changes

theresa-m force-pushed the fix_1520_tests branch from bb11904 to 424b1f7 Compare April 13, 2018 18:06

theresa-m changed the title ~~Tests for isAccessUserOnly0 native~~ WIP: Tests for isAccessUserOnly0 native Apr 13, 2018

theresa-m force-pushed the fix_1520_tests branch 3 times, most recently from 98661b1 to 5ab7fcf Compare April 23, 2018 14:03

theresa-m changed the title ~~WIP: Tests for isAccessUserOnly0 native~~ Tests for isAccessUserOnly0 native May 17, 2018

theresa-m force-pushed the fix_1520_tests branch from 5ab7fcf to 6ea5838 Compare May 17, 2018 23:01

pshipton approved these changes May 19, 2018

View reviewed changes

llxia reviewed May 22, 2018

View reviewed changes

llxia approved these changes May 22, 2018

View reviewed changes

theresa-m added 2 commits May 22, 2018 11:10

Tests for isAccessUserOnly0 native

79e2a4e

- includes tests for isAccessUserOnly0 in Java8 and Java9 - expanded test infra to have src_90_current and src_90_latest files for different JCL versions in Java 9 Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>

Remove unnecessary test debug statement

ff96871

Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>

theresa-m force-pushed the fix_1520_tests branch from 6ea5838 to ff96871 Compare May 22, 2018 15:13

pshipton merged commit 8d42939 into eclipse-openj9:master May 23, 2018

theresa-m deleted the fix_1520_tests branch June 5, 2018 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests for isAccessUserOnly0 native #1656

Tests for isAccessUserOnly0 native #1656

theresa-m commented Apr 11, 2018 •

edited

Loading

theresa-m Apr 11, 2018

smlambert Apr 11, 2018

smlambert Apr 11, 2018

smlambert Apr 11, 2018

smlambert commented Apr 11, 2018

DanHeidinga commented Apr 11, 2018

DanHeidinga commented Apr 11, 2018

llxia Apr 11, 2018 •

edited

Loading

theresa-m commented Apr 11, 2018

theresa-m commented Apr 11, 2018

theresa-m commented Apr 13, 2018

llxia Apr 13, 2018

theresa-m Apr 13, 2018

llxia commented Apr 13, 2018

theresa-m commented Apr 13, 2018

llxia commented Apr 13, 2018

theresa-m commented May 17, 2018

theresa-m commented May 17, 2018

theresa-m commented May 18, 2018 •

edited

Loading

pshipton commented May 18, 2018

pshipton commented May 18, 2018

theresa-m commented May 18, 2018

pshipton commented May 19, 2018

pshipton commented May 19, 2018

llxia May 22, 2018

theresa-m May 22, 2018

llxia left a comment

theresa-m commented May 22, 2018

theresa-m commented May 23, 2018

Tests for isAccessUserOnly0 native #1656

Tests for isAccessUserOnly0 native #1656

Conversation

theresa-m commented Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smlambert commented Apr 11, 2018

DanHeidinga commented Apr 11, 2018

DanHeidinga commented Apr 11, 2018

llxia Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

theresa-m commented Apr 11, 2018

theresa-m commented Apr 11, 2018

theresa-m commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llxia commented Apr 13, 2018

theresa-m commented Apr 13, 2018

llxia commented Apr 13, 2018

theresa-m commented May 17, 2018

theresa-m commented May 17, 2018

theresa-m commented May 18, 2018 • edited Loading

pshipton commented May 18, 2018

pshipton commented May 18, 2018

theresa-m commented May 18, 2018

pshipton commented May 19, 2018

pshipton commented May 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llxia left a comment

Choose a reason for hiding this comment

theresa-m commented May 22, 2018

theresa-m commented May 23, 2018

theresa-m commented Apr 11, 2018 •

edited

Loading

llxia Apr 11, 2018 •

edited

Loading

theresa-m commented May 18, 2018 •

edited

Loading