keep JSyntaxTextArea text value for use in headless mode by stokpop · Pull Request #696 · apache/jmeter

stokpop · 2022-01-25T13:47:56Z

Description

Keep the text that is set on overridden JSyntaxTextArea anonymous class in headless mode.

Motivation and Context

Working on a project that uses jMeter components in a headless way (java.awt.headless=true), we found that the setText(...) is not kept on the JSyntaxTextArea from getInstance(...). Following getText() call returns null. With java.awt.headless=false, to try and run with the original JSyntaxTextArea on headless linux it fails as there is no X11 display.

This might benefit headless unit test as well.

How Has This Been Tested?

Unit tests added. Snapshot build works in project.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code follows the [code style][style-guide] of this project.
I have updated the documentation accordingly (javadocs and code comments)

vlsi · 2022-01-25T14:13:08Z

src/core/src/main/java/org/apache/jmeter/gui/util/JSyntaxTextArea.java

                    public void discardAllEdits() { }
                    @Override
-                    public void setText(String t) { }
+                    public void setText(String t) {


Just wondering, does the removal of setText override solves the issue?

If I add a super.setText(t) here, I get the following stacktrace:

java.lang.NullPointerException at org.fife.ui.rsyntaxtextarea.RSyntaxTextArea.getFontMetricsForTokenType(RSyntaxTextArea.java:1204) at org.fife.ui.rsyntaxtextarea.TokenImpl.getWidthUpTo(TokenImpl.java:578) at org.fife.ui.rsyntaxtextarea.TokenImpl.getWidth(TokenImpl.java:570) at org.fife.ui.rsyntaxtextarea.WrappedSyntaxView.calculateBreakPosition(WrappedSyntaxView.java:127) at org.fife.ui.rsyntaxtextarea.WrappedSyntaxView$WrappedLine.calculateLineCount(WrappedSyntaxView.java:1191) at org.fife.ui.rsyntaxtextarea.WrappedSyntaxView$WrappedLine.handleDocumentEvent(WrappedSyntaxView.java:1440) at org.fife.ui.rsyntaxtextarea.WrappedSyntaxView$WrappedLine.insertUpdate(WrappedSyntaxView.java:1464) at org.fife.ui.rsyntaxtextarea.WrappedSyntaxView.insertUpdate(WrappedSyntaxView.java:705) at javax.swing.plaf.basic.BasicTextUI$RootView.insertUpdate(BasicTextUI.java:1610) at javax.swing.plaf.basic.BasicTextUI$UpdateHandler.insertUpdate(BasicTextUI.java:1869) at javax.swing.text.AbstractDocument.fireInsertUpdate(AbstractDocument.java:201) at org.fife.ui.rsyntaxtextarea.RSyntaxDocument.fireInsertUpdate(RSyntaxDocument.java:187) at javax.swing.text.AbstractDocument.handleInsertString(AbstractDocument.java:748) at javax.swing.text.AbstractDocument.insertString(AbstractDocument.java:707) at javax.swing.text.PlainDocument.insertString(PlainDocument.java:130) at javax.swing.text.AbstractDocument.replace(AbstractDocument.java:669) at javax.swing.text.JTextComponent.setText(JTextComponent.java:1669) at org.apache.jmeter.gui.util.JSyntaxTextArea$1.setText(JSyntaxTextArea.java:122)

So, I would say, no, it does not resolve the issue :)

True, I saw same NPE.

FSchumacher · 2022-01-26T16:30:39Z

src/core/src/main/java/org/apache/jmeter/gui/util/JSyntaxTextArea.java

                    public void discardAllEdits() { }
                    @Override
-                    public void setText(String t) { }
+                    public void setText(String t) {


If I add a super.setText(t) here, I get the following stacktrace:

java.lang.NullPointerException at org.fife.ui.rsyntaxtextarea.RSyntaxTextArea.getFontMetricsForTokenType(RSyntaxTextArea.java:1204) at org.fife.ui.rsyntaxtextarea.TokenImpl.getWidthUpTo(TokenImpl.java:578) at org.fife.ui.rsyntaxtextarea.TokenImpl.getWidth(TokenImpl.java:570) at org.fife.ui.rsyntaxtextarea.WrappedSyntaxView.calculateBreakPosition(WrappedSyntaxView.java:127) at org.fife.ui.rsyntaxtextarea.WrappedSyntaxView$WrappedLine.calculateLineCount(WrappedSyntaxView.java:1191) at org.fife.ui.rsyntaxtextarea.WrappedSyntaxView$WrappedLine.handleDocumentEvent(WrappedSyntaxView.java:1440) at org.fife.ui.rsyntaxtextarea.WrappedSyntaxView$WrappedLine.insertUpdate(WrappedSyntaxView.java:1464) at org.fife.ui.rsyntaxtextarea.WrappedSyntaxView.insertUpdate(WrappedSyntaxView.java:705) at javax.swing.plaf.basic.BasicTextUI$RootView.insertUpdate(BasicTextUI.java:1610) at javax.swing.plaf.basic.BasicTextUI$UpdateHandler.insertUpdate(BasicTextUI.java:1869) at javax.swing.text.AbstractDocument.fireInsertUpdate(AbstractDocument.java:201) at org.fife.ui.rsyntaxtextarea.RSyntaxDocument.fireInsertUpdate(RSyntaxDocument.java:187) at javax.swing.text.AbstractDocument.handleInsertString(AbstractDocument.java:748) at javax.swing.text.AbstractDocument.insertString(AbstractDocument.java:707) at javax.swing.text.PlainDocument.insertString(PlainDocument.java:130) at javax.swing.text.AbstractDocument.replace(AbstractDocument.java:669) at javax.swing.text.JTextComponent.setText(JTextComponent.java:1669) at org.apache.jmeter.gui.util.JSyntaxTextArea$1.setText(JSyntaxTextArea.java:122)

So, I would say, no, it does not resolve the issue :)

FSchumacher · 2022-01-26T16:32:01Z

src/core/src/main/java/org/apache/jmeter/gui/action/Save.java

This has nothing to do with the description of the issue. I don't think it is needed here and should be done in it's own PR (if at all).

true, typo unrelated to PR, removed it

FSchumacher · 2022-01-26T16:33:26Z

src/core/src/test/java/org/apache/jmeter/gui/util/JSyntaxTextAreaTest.java

        } catch (HeadlessException he) {
-            // Does not work in headless mode
+            // Does not work in headless mode, which depends on value of java.awt.headless property
+            // and the OS (e.g. might work on MacOS and not on Linux due to missing X11).


I don't understand this new comment. Can you explain it to me?

How I got here: I have Kotlin code (to generate a jmx file based on open-api spec) that calls setText() and getText() for the json payload. When running from Kotlin code on MacOS laptop, the code runs fine in Intellij: getText() returns a value. When running it from Kotlin script, the getText() returns null after setText(). Why? Debugging shows that java.awt.headless=true when running Kotlin script. When running Kotlin code the java.awt.headless property is not present, so does not run headless. (In that case it I guess it might work because awt.toolkit=sun.lwawt.macosx.LWCToolkit is present). Then on linux docker (target system) running non-headless does not work because it says no DISPLAY is set (awt.toolkit is missing?). And running headless (with java.awt.headless=true), the getText() returns null.

I've filed bobbylight/RSyntaxTextArea#424 to RSyntaxTextArea. I hope we can remove headless workaround then.

FSchumacher · 2022-01-26T16:35:53Z

src/core/src/test/java/org/apache/jmeter/gui/util/JSyntaxTextAreaTest.java

+        String key = "java.awt.headless";
+        String initialValue = System.getProperty(key);
+        try {
+            System.setProperty(key, "true");


Is this really a sane way to test headless mode?

Maybe there is a better way? It is indeed tricky as it might impact other tests, that's why the @Isolated is added. The code directly depends on the property value in its behaviour. I've seen some mock framework code that overrides the System.getProperty() to return a certain value, but there might be classloading issues.

For the other tests, we are expecting the test suite to be set up in the correct way (for example most of our CI stuff is done in a headless setup). But if this works, we can use it.

FSchumacher · 2022-01-26T16:36:39Z

src/core/src/test/java/org/apache/jmeter/gui/util/JSyntaxTextAreaTest.java

+            JSyntaxTextArea textArea = JSyntaxTextArea.getInstance(10,20);
+
+            String myText = "my text";
+            textArea.setText(myText);


Are you testing set/getText here, or the creation of a specialised subclass?

Test if the textArea returned by getInstance() in headless mode is retaining the text. Without the fix of the specialised subclass this will fail, because getText() returns null.

Right, but why the call textArea.setCodeFoldingEnabled(true);? It seems that you want to test, that we get a special variant, that works in headless mode. But in my eyes, you know, that it works in headless mode (or could not check it anyway) and this test is about saving the text. One test should assert one thing (at least new ones should :))

agree, I removed the NPE implicit check

FSchumacher · 2022-01-26T16:37:46Z

src/core/src/test/java/org/apache/jmeter/gui/util/JSyntaxTextAreaTest.java

Is this expected? I would expect, that this fail should never be reached and should point this out a bit more clearly.

The exception is not expected here, because the explicit java.awt.headless=true, so the fail. This is indeed different from the other test, that seems to allow the HeadlessException by ignoring it. I can change fail text.

This reverts commit 8de2c5f.

…eadless mode.

codecov-commenter · 2022-01-26T22:29:22Z

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 55.58%. Comparing base (85580b4) to head (6ea80ac).
Report is 701 commits behind head on master.

Files	Patch %	Lines
...va/org/apache/jmeter/gui/util/JSyntaxTextArea.java	0.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #696      +/-   ##
============================================
- Coverage     55.59%   55.58%   -0.01%     
  Complexity    10336    10336              
============================================
  Files          1059     1059              
  Lines         65050    65056       +6     
  Branches       7401     7401              
============================================
  Hits          36163    36163              
- Misses        26337    26343       +6     
  Partials       2550     2550

Files	Coverage Δ
...va/org/apache/jmeter/gui/util/JSyntaxTextArea.java	`59.57% <0.00%> (-1.97%)`	⬇️

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85580b4...6ea80ac. Read the comment docs.

FSchumacher · 2022-01-30T11:40:46Z

src/core/src/test/java/org/apache/jmeter/gui/util/JSyntaxTextAreaTest.java

I think we could drop this catch, as every exception here is unexpected and should be seen after.

Agree, I dropped the catch.

stokpop added 4 commits January 17, 2022 16:15

keep JSyntaxTextArea text value for use in headless mode

94f5664

unit tests for headless JSyntaxTextArea fix

97bb87f

typo fix

8de2c5f

Merge branch 'apache:master' into minor-fixes

edece34

vlsi reviewed Jan 25, 2022

View reviewed changes

FSchumacher reviewed Jan 26, 2022

View reviewed changes

stokpop added 4 commits January 26, 2022 20:22

Revert "typo fix"

8206129

This reverts commit 8de2c5f.

Improved fail text.

16e2930

Fix java style format issue.

9a37684

Remove multiple assert from test. Just check if text is retained in h…

9f50461

…eadless mode.

vlsi mentioned this pull request Jan 27, 2022

HeadlessException when instantiating RSyntaxTextArea in headless mode bobbylight/RSyntaxTextArea#424

Closed

FSchumacher reviewed Jan 30, 2022

View reviewed changes

dropped HeadlessException catch from unit test

6ea80ac

asfgit closed this in 2ca7cf9 Mar 9, 2022

Conversation

stokpop commented Jan 25, 2022

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

codecov-commenter commented Jan 26, 2022 •

edited

Loading