WIP test unit for: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV #123

ronyfla · 2020-02-22T15:41:41Z

…59: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Download

$ git fetch https://git.openjdk.java.net/jfx pull/123/head:pull/123
$ git checkout pull/123

…59: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV

bridgekeeper · 2020-02-22T15:42:47Z

Hi ronyfla, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user ronyfla" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

ronyfla · 2020-02-22T16:31:21Z

/signed

bridgekeeper · 2020-02-22T16:31:51Z

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

ronyfla · 2020-02-23T14:55:45Z

OK, forgot to submit an explanatory text related to this fix. Posted [1] which explains the problem and the suggested solution for discussion.

[1] 'Ad suggested test unit for "JDK-8234959 FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV"': https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-February/025104.html

ronyfla · 2020-02-23T15:02:12Z

As this suggested test unit gets added to the 'systemTests' one can run that particular test by issuing:

./gradlew -PFULL_TEST=true :systemTests:test --tests ModuleLauncherTest

kevinrushforth · 2020-02-26T22:36:38Z

I have two overall comments before I review the specifics:

The overall structure of this test looks good. I can also confirm that it fails without your fix and passes with your fix. So I think it's in good enough shape to review. Which brings me to my next point...
We require that each pull request be associated with a unique bug. So this test should be added to PR 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV #122 as an additional commit.

I will have a few specific comments on the test, so if you want to wait to add the test to PR #122 that would be fine.

kevinrushforth · 2020-02-26T22:41:01Z

As an FYI, you can run just the one test method you added as follows:

gradle -PFULL_TEST=true :systemTests:test --tests ModuleLauncherTest.testModuleFXMLScriptTest

kevinrushforth

I like the test quite a bit, and for the most part it only needs cosmetic changes.

I marked this as "changes needed" because it needs to be folded into PR #122. Once you do that, you can close this WIP PR.

kevinrushforth · 2020-02-26T22:45:03Z

tests/system/src/test/java/test/launchertest/ModuleLauncherTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2018, 2020, Oracle and/or its affiliates. All rights reserved.


Only the year of creation and the year last modified should be listed, so that should be:

* Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved.

kevinrushforth · 2020-02-26T22:51:42Z

tests/system/src/test/java/test/launchertest/ModuleLauncherTest.java

@@ -274,4 +276,9 @@ public void testModuleFXMLQualOpened() throws Exception {
        doTestLaunchModule(modulePath6, "myapp6.AppFXMLQualOpened");
    }

+    @Test (timeout = 15000)
+    public void testModuleFXMLScriptTest() throws Exception {


I recommend a method name that doesn't start with testModuleFXML, since it otherwise when looking at a list of tests (e.g., in the test report), it lands in the middle of a set of related tests to which this new test is unrelated. This test really isn't testing modules, so maybe something with "module" in the name?

I meant without module in the name...

kevinrushforth · 2020-02-26T22:52:33Z

tests/system/src/testscriptapp1/java/mymod/module-info.java

@@ -0,0 +1,9 @@
+module mymod {


This file needs a copyright header

kevinrushforth · 2020-02-26T22:54:57Z

tests/system/src/testscriptapp1/java/mymod/myapp1/Constants.java

@@ -0,0 +1,37 @@
+/*
+ * Copyright (c) 2017-2019, Oracle and/or its affiliates. All rights reserved.


Years should be 2017, 2020,

kevinrushforth · 2020-02-26T22:56:46Z

tests/system/src/testscriptapp1/java/mymod/myapp1/FXMLScriptTest.java

@@ -0,0 +1,303 @@
+/*


This test app should be called something without Test in the name, since it isn't run by the gradle unit testing framework.

kevinrushforth · 2020-02-26T23:26:49Z

tests/system/src/testscriptapp1/java/mymod/pseudoScriptEngine/RgfPseudoScriptEngineFactory.java

+
+
+public class RgfPseudoScriptEngineFactory implements ScriptEngineFactory
+{


Curly brace on previous line.

kevinrushforth · 2020-02-26T23:28:03Z

tests/system/src/testscriptapp1/java/mymod/pseudoScriptEngine/RgfPseudoScriptEngineFactory.java

+    final static String       ENGINE_NAME       = "RgfPseudoScriptLanguage (RPSL) 1.0.0";
+    final static String       SHORT_ENGINE_NAME = "rpsl";
+    final static String       ENGINE_VERSION    = "100.20200220";
+    final static List<String> EXTENSIONS        = Arrays.asList("rpsl", "RPSL" );
+    final static String       LANGUAGE_NAME     = "RgfPseudoScriptLanguage";
+    final static String       LANGUAGE_VERSION  = "1.0.0.100.20200220";
+
+    final static List<String> MIME_TYPES        = Arrays.asList("text/rpsl", "application/x-rpsl");
+    final static String       THREADING         = "MULTITHREADED";
+
+    final static List<String> ENGINE_NAMES = Arrays.asList(SHORT_ENGINE_NAME, "RgfPseudoSL");


Here is an example where I think it is fine to line them up. Can you also do that for the last of these?

Also, our general pattern is static final (static goes first). I probably missed noting that in other places.

kevinrushforth · 2020-02-26T23:28:52Z

tests/system/src/testscriptapp1/java/mymod/pseudoScriptEngine/RgfPseudoScriptEngineFactory.java

+
+    public String getOutputStatement(String toDisplay) {
+        String tmpDisplay=toStringLiteral(toDisplay);
+        return "say "+tmpDisplay+" /* Rexx style (duplicate quotes within string) */ ";


spaces around the +

kevinrushforth · 2020-02-26T23:29:43Z

...system/src/testscriptapp1/resources/mymod/META-INF/services/javax.script.ScriptEngineFactory

@@ -0,0 +1,2 @@
+pseudoScriptEngine.RgfPseudoScriptEngineFactory
+


Is this extra blank line needed?

kevinrushforth · 2020-02-26T23:31:23Z

tests/system/src/testscriptapp1/resources/mymod/myapp1/demo_01.fxml

@@ -0,0 +1,42 @@
+<?xml version="1.0" encoding="UTF-8"?>
+


Please add a copyright header here. See any of the other .fxml file in tests/system for an example.

ronyfla · 2020-02-27T14:25:11Z

Did apply the changes to the WIP, such that you could look over them here.
Also, afterwards I incorporated it into # 122 per your suggestion, so this WIP could be closed/deleted.

ronyfla · 2020-02-28T11:56:36Z

This test unit - after applying the review comments - got merged to https://github.com/openjdk/jfx/pull/122/commits, therefore closing this WIP.

test for <https://bugs.openjdk.java.net/browse/JDK-8234959> JDK-82349…

e251996

…59: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV

bridgekeeper bot added the oca Needs verification of OCA signatory status label Feb 22, 2020

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Feb 22, 2020

Removed/replaced white space.

6e4df77

ronyfla requested a review from kevinrushforth February 23, 2020 14:58

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Feb 25, 2020

kevinrushforth requested changes Feb 26, 2020

View reviewed changes

kevinrushforth mentioned this pull request Feb 26, 2020

8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV #122

Closed

3 tasks

ronyfla added 2 commits February 27, 2020 15:05

Incorporating Kevin Rushfort's review comments to this WIP.

8cf0a6e

Incorporating Kevin Rushfort's review comments to this WIP.

2db3171

ronyfla closed this Feb 28, 2020

ronyfla deleted the scripttest branch February 28, 2020 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP test unit for: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV #123

WIP test unit for: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV #123

ronyfla commented Feb 22, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 22, 2020

ronyfla commented Feb 22, 2020

bridgekeeper bot commented Feb 22, 2020

ronyfla commented Feb 23, 2020

ronyfla commented Feb 23, 2020

kevinrushforth commented Feb 26, 2020

kevinrushforth commented Feb 26, 2020

kevinrushforth left a comment

kevinrushforth Feb 26, 2020

kevinrushforth Feb 26, 2020

kevinrushforth Feb 26, 2020

kevinrushforth Feb 26, 2020

kevinrushforth Feb 26, 2020

kevinrushforth Feb 26, 2020

kevinrushforth Feb 26, 2020

kevinrushforth Feb 26, 2020

kevinrushforth Feb 26, 2020

kevinrushforth Feb 26, 2020

kevinrushforth Feb 26, 2020

ronyfla commented Feb 27, 2020

ronyfla commented Feb 28, 2020

		@@ -0,0 +1,37 @@
		/*
		* Copyright (c) 2017-2019, Oracle and/or its affiliates. All rights reserved.



		public class RgfPseudoScriptEngineFactory implements ScriptEngineFactory
		{

		@@ -0,0 +1,2 @@
		pseudoScriptEngine.RgfPseudoScriptEngineFactory

WIP test unit for: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV #123

WIP test unit for: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV #123

Conversation

ronyfla commented Feb 22, 2020 • edited by openjdk bot Loading

Progress

Download

bridgekeeper bot commented Feb 22, 2020

ronyfla commented Feb 22, 2020

bridgekeeper bot commented Feb 22, 2020

ronyfla commented Feb 23, 2020

ronyfla commented Feb 23, 2020

kevinrushforth commented Feb 26, 2020

kevinrushforth commented Feb 26, 2020

kevinrushforth left a comment

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

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

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

ronyfla commented Feb 27, 2020

ronyfla commented Feb 28, 2020

ronyfla commented Feb 22, 2020 •

edited by openjdk bot

Loading