Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NumberFormatException when counter is empty or not a digit on Proxy Settings panel #5901

Merged
merged 1 commit into from May 10, 2023

Conversation

milamberspace
Copy link
Contributor

@milamberspace milamberspace commented May 10, 2023

Description

On proxy settings, if the Counter is empty or not a numeric value, when you click on Set Counter button, JMeter have a NumberFormatException

2023-05-04 17:10:42,819 ERROR o.a.j.JMeter: Uncaught exception in thread Thread[AWT-EventQueue-0,6,main]
java.lang.NumberFormatException: For input string: ""
at java.lang.NumberFormatException.forInputString(NumberFormatException.java:67) ~[?:?]
at java.lang.Integer.parseInt(Integer.java:678) ~[?:?]
at java.lang.Integer.parseInt(Integer.java:786) ~[?:?]
at org.apache.jmeter.protocol.http.proxy.gui.ProxyControlGui.lambda$createHTTPSamplerPanel$1(ProxyControlGui.java:985) ~[ApacheJMeter_http.jar:5.4.3]

Motivation and Context

This PR fix this issue with a test on the value, and display a popup if not a digit value

How Has This Been Tested?

  • Enter text or leave empty the counter field and click on Set Counter button

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • [ X] My code follows the [code style][style-guide] of this project.
  • I have updated the documentation accordingly.

Comment on lines 705 to 708
JOptionPane.showMessageDialog(this,
//"Counter isn't a number value.", "Error on counter value", JOptionPane.WARNING_MESSAGE);
JMeterUtils.getResString("proxy_settings_counter_error_digits"), // $NON-NLS-1$
JMeterUtils.getResString("proxy_settings_counter_error_invalid_data"), // $NON-NLS-1$
JOptionPane.WARNING_MESSAGE);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the problematic value to the error message?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it support ${...} expressions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vlsi what is ${...} expressions? I don't understand
Not sure that add the problematic value is needed, specially if the value is empty. IMO only display a warning is ok.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. something like ${__P(staringValue)}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see the number was already parsed without evaluating the properties, so supporting ${..} there would be a different issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems usual to not evaluate the value against the JMeter function for the Proxy settings. Proxy is use only on GUI mode, not in load tests, so it's not need to evaluate the value if ${...} expression is present. Are you ok with this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no use case for evaluation ${..} there yet, however, I think having uniform support for ${..} would be nice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR aims first to correct the NFE (which is a technical error), and not to make improvements in the Counter field. If there is a need to add functionality for evaluating JMeter functions, that will be the subject of another ticket. Can I have your LGTM?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works for me. However, would you please include the problematic value itself in the error message?

@vlsi vlsi changed the title Fix NumberFormatException when counter is empty or not a digit on Pro… Fix NumberFormatException when counter is empty or not a digit on Proxy Settings panel May 10, 2023
asfgit pushed a commit that referenced this pull request May 10, 2023
… is empty or not a digit on Proxy Settings panel)
@asfgit asfgit merged commit 7f9219c into apache:master May 10, 2023
4 checks passed
@milamberspace milamberspace deleted the Fix_NFE_on_counter_button branch May 10, 2023 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants