You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by hitesh22 <gi...@git.apache.org> on 2018/10/27 22:05:53 UTC
[GitHub] jmeter pull request #413: Enhancement 62829 - Allow specifying proxy scheme ...
GitHub user hitesh22 opened a pull request:
https://github.com/apache/jmeter/pull/413
Enhancement 62829 - Allow specifying proxy scheme to use
## Description
<!--- Provide a general summary of your changes in the Title above -->
<!--- Describe your changes in detail here -->
A proxy could be running using https but currently there is no way
to specify it. This enhancement allows user to specify proxy scheme to use.
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
https://bz.apache.org/bugzilla/show_bug.cgi?id=62829
## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, tests ran to see how -->
<!--- your change affects other areas of the code, etc. -->
ant package-and-check
ant test
Tested using ant run_gui
Tested on Mac and using on Ubuntu headless tests.
Using these changes with a proxy server using https
## Screenshots (if appropriate):
Attached to bugzilla ticket.
## Types of changes
<!--- What types of changes does your code introduce? Delete as appropriate -->
- New feature (non-breaking change which adds functionality)
## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [ x] My code follows the [code style][style-guide] of this project.
- [ ] I have updated the documentation accordingly.
[style-guide]: https://wiki.apache.org/jmeter/CodeStyleGuidelines
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hitesh22/jmeter 62829
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/jmeter/pull/413.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #413
----
commit 5f5b5318f98f0c6e74d75326b5c653c72f2919aa
Author: hitesh <hp...@...>
Date: 2018-10-27T19:01:44Z
Enhancement 62829 - Allow specifying proxy scheme to use
Motivation and Context
A proxy could be running using https but currently there is no way
to specify it. This enhancement allows user to specify proxy scheme to use.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62829
How Has This Been Tested?
Tested using ant run_gui
Tested on Mac and using on Ubuntu headless tests.
Only added functionality to code
Types of changes
Enhancement (non-breaking change which fixes an issue)
Checklist:
[x ] My code follows the code style of this project.
----
---
[GitHub] jmeter issue #413: Enhancement 62829 - Allow specifying proxy scheme to use
Posted by hitesh22 <gi...@git.apache.org>.
Github user hitesh22 commented on the issue:
https://github.com/apache/jmeter/pull/413
@pmouawad There is a test failing for label missing for French message properties file. Do I need to fix this? What is the process now for getting my changes accepted for merge? Do I have to find all the translations for the different languages and submit it with my changes?
Appreciate any guidance you can provide to help get these changes accepted into next release. Thanks.
1) checkI18n(org.apache.jmeter.resources.PackageTest)
[java] junit.framework.AssertionFailedError: 1 missing labels, labels missing:Missing labels in bundle:org/apache/jmeter/resources/messages_fr.properties
---
[GitHub] jmeter issue #413: Enhancement 62829 - Allow specifying proxy scheme to use
Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:
https://github.com/apache/jmeter/pull/413
Thanks for updating your PR to add the cmd line option.
Yes french labels are also mandatory, can you update it (I would correct it if needed) so that we see if all tests pass.
Once done, we’ll merge it.
Thanks for contributing !
---
[GitHub] jmeter pull request #413: Enhancement 62829 - Allow specifying proxy scheme ...
Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/413#discussion_r228741947
--- Diff: src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPHC4Impl.java ---
@@ -1037,7 +1044,7 @@ private CloseableHttpClient setupClient(JMeterVariables jMeterVariables, URL url
// Set up proxy details
if(useProxy) {
- HttpHost proxy = new HttpHost(proxyHost, proxyPort);
+ HttpHost proxy = new HttpHost(proxyHost, proxyPort, proxyScheme);
--- End diff --
If proxyScheme is empty, then it should be passed as null so that default (HTTP) is used.
---
[GitHub] jmeter issue #413: Enhancement 62829 - Allow specifying proxy scheme to use
Posted by hitesh22 <gi...@git.apache.org>.
Github user hitesh22 commented on the issue:
https://github.com/apache/jmeter/pull/413
> Thanks for patch update.
> Thinking more about it **and to be coherent** we should also add this as a command line option as per:
>
> * http://jmeter.apache.org/usermanual/get-started.html#proxy_server
>
> Do you think you can update your PR ?
> Thanks
Probably. I will take a look.
---
[GitHub] jmeter pull request #413: Enhancement 62829 - Allow specifying proxy scheme ...
Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/413#discussion_r234444500
--- Diff: src/core/org/apache/jmeter/JMeter.java ---
@@ -746,13 +751,15 @@ private void setProxy(CLArgsParser parser) throws IllegalUserActionException {
}
}
if (parser.getArgumentById(PROXY_HOST) != null && parser.getArgumentById(PROXY_PORT) != null) {
+ String scheme = parser.getArgumentById(PROXY_SCHEME).getArgument();
String h = parser.getArgumentById(PROXY_HOST).getArgument();
String p = parser.getArgumentById(PROXY_PORT).getArgument();
+ System.setProperty("http.proxyScheme", scheme );// $NON-NLS-1$
--- End diff --
Should be set only if not blank/null
---
[GitHub] jmeter pull request #413: Enhancement 62829 - Allow specifying proxy scheme ...
Posted by hitesh22 <gi...@git.apache.org>.
Github user hitesh22 commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/413#discussion_r229147047
--- Diff: src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPHC4Impl.java ---
@@ -1037,7 +1044,7 @@ private CloseableHttpClient setupClient(JMeterVariables jMeterVariables, URL url
// Set up proxy details
if(useProxy) {
- HttpHost proxy = new HttpHost(proxyHost, proxyPort);
+ HttpHost proxy = new HttpHost(proxyHost, proxyPort, proxyScheme);
--- End diff --
@pmouawad
Added above at line 950: proxyScheme = StringUtils.isBlank(proxyScheme) ? null : proxyScheme;
---
[GitHub] jmeter issue #413: Enhancement 62829 - Allow specifying proxy scheme to use
Posted by codecov-io <gi...@git.apache.org>.
Github user codecov-io commented on the issue:
https://github.com/apache/jmeter/pull/413
# [Codecov](https://codecov.io/gh/apache/jmeter/pull/413?src=pr&el=h1) Report
> Merging [#413](https://codecov.io/gh/apache/jmeter/pull/413?src=pr&el=desc) into [trunk](https://codecov.io/gh/apache/jmeter/commit/adf3bcb8be6b57479768c1114a8fb9138138f27a?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `79.41%`.
[](https://codecov.io/gh/apache/jmeter/pull/413?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## trunk #413 +/- ##
===========================================
+ Coverage 58.59% 58.6% +0.01%
- Complexity 10629 10635 +6
===========================================
Files 1196 1196
Lines 76007 76039 +32
Branches 7348 7349 +1
===========================================
+ Hits 44535 44563 +28
- Misses 28966 28971 +5
+ Partials 2506 2505 -1
```
| [Impacted Files](https://codecov.io/gh/apache/jmeter/pull/413?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...jmeter/protocol/http/sampler/HTTPAbstractImpl.java](https://codecov.io/gh/apache/jmeter/pull/413/diff?src=pr&el=tree#diff-c3JjL3Byb3RvY29sL2h0dHAvb3JnL2FwYWNoZS9qbWV0ZXIvcHJvdG9jb2wvaHR0cC9zYW1wbGVyL0hUVFBBYnN0cmFjdEltcGwuamF2YQ==) | `55.91% <100%> (+0.47%)` | `35 <1> (+1)` | :arrow_up: |
| [.../jmeter/protocol/http/sampler/HTTPSamplerBase.java](https://codecov.io/gh/apache/jmeter/pull/413/diff?src=pr&el=tree#diff-c3JjL3Byb3RvY29sL2h0dHAvb3JnL2FwYWNoZS9qbWV0ZXIvcHJvdG9jb2wvaHR0cC9zYW1wbGVyL0hUVFBTYW1wbGVyQmFzZS5qYXZh) | `62.12% <100%> (+0.04%)` | `194 <1> (+1)` | :arrow_up: |
| [...ache/jmeter/protocol/http/sampler/HTTPHC4Impl.java](https://codecov.io/gh/apache/jmeter/pull/413/diff?src=pr&el=tree#diff-c3JjL3Byb3RvY29sL2h0dHAvb3JnL2FwYWNoZS9qbWV0ZXIvcHJvdG9jb2wvaHR0cC9zYW1wbGVyL0hUVFBIQzRJbXBsLmphdmE=) | `66.3% <37.5%> (-0.28%)` | `88 <0> (ø)` | |
| [...eter/protocol/http/config/gui/HttpDefaultsGui.java](https://codecov.io/gh/apache/jmeter/pull/413/diff?src=pr&el=tree#diff-c3JjL3Byb3RvY29sL2h0dHAvb3JnL2FwYWNoZS9qbWV0ZXIvcHJvdG9jb2wvaHR0cC9jb25maWcvZ3VpL0h0dHBEZWZhdWx0c0d1aS5qYXZh) | `81.32% <91.66%> (+0.54%)` | `22 <1> (+1)` | :arrow_up: |
| [...r/protocol/http/control/gui/HttpTestSampleGui.java](https://codecov.io/gh/apache/jmeter/pull/413/diff?src=pr&el=tree#diff-c3JjL3Byb3RvY29sL2h0dHAvb3JnL2FwYWNoZS9qbWV0ZXIvcHJvdG9jb2wvaHR0cC9jb250cm9sL2d1aS9IdHRwVGVzdFNhbXBsZUd1aS5qYXZh) | `86.02% <91.66%> (+0.31%)` | `25 <1> (+1)` | :arrow_up: |
| [...c/core/org/apache/jmeter/reporters/Summariser.java](https://codecov.io/gh/apache/jmeter/pull/413/diff?src=pr&el=tree#diff-c3JjL2NvcmUvb3JnL2FwYWNoZS9qbWV0ZXIvcmVwb3J0ZXJzL1N1bW1hcmlzZXIuamF2YQ==) | `86.25% <0%> (+1.52%)` | `19% <0%> (+2%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/jmeter/pull/413?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/jmeter/pull/413?src=pr&el=footer). Last update [adf3bcb...5f5b531](https://codecov.io/gh/apache/jmeter/pull/413?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
---
[GitHub] jmeter pull request #413: Enhancement 62829 - Allow specifying proxy scheme ...
Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on a diff in the pull request:
https://github.com/apache/jmeter/pull/413#discussion_r234444521
--- Diff: docs/usermanual/get-started.html ---
@@ -930,6 +930,11 @@ <h2 id="proxy_server">1.4.3 Using JMeter behind a proxy<a class="sectionlink" hr
<dl>
+<dt>
--- End diff --
To update documentation, update xdocs/usermanual/get-started.xml as html are generated from xml, thanks
---
[GitHub] jmeter pull request #413: Enhancement 62829 - Allow specifying proxy scheme ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/jmeter/pull/413
---
[GitHub] jmeter issue #413: Enhancement 62829 - Allow specifying proxy scheme to use
Posted by pmouawad <gi...@git.apache.org>.
Github user pmouawad commented on the issue:
https://github.com/apache/jmeter/pull/413
Thanks for patch update.
Thinking more about it **and to be coherent** we should also add this as a command line option as per:
- http://jmeter.apache.org/usermanual/get-started.html#proxy_server
Do you think you can update your PR ?
Thanks
---