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 62887. Customized SampleLabel is ignored and overwritten by JMet… #429

Closed
wants to merge 9 commits into from

Conversation

artem-fedorov
Copy link

…er 5.0

@codecov-io
Copy link

Codecov Report

Merging #429 into trunk will increase coverage by <.01%.
The diff coverage is 72%.

Impacted file tree graph

@@             Coverage Diff             @@
##              trunk    #429      +/-   ##
===========================================
+ Coverage     58.59%   58.6%   +<.01%     
- Complexity    10632   10637       +5     
===========================================
  Files          1196    1196              
  Lines         76051   76072      +21     
  Branches       7357    7357              
===========================================
+ Hits          44562   44579      +17     
- Misses        28981   28984       +3     
- Partials       2508    2509       +1
Impacted Files Coverage Δ Complexity Δ
...c/org/apache/jmeter/samplers/TestSampleResult.java 94.3% <100%> (+0.44%) 25 <1> (+1) ⬆️
.../core/org/apache/jmeter/samplers/SampleResult.java 74.66% <36.36%> (-0.98%) 135 <4> (+2)
...c/core/org/apache/jmeter/reporters/Summariser.java 86.25% <0%> (+1.52%) 19% <0%> (+2%) ⬆️

Continue to review full report at Codecov.

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

@@ -2,16 +2,16 @@
<testResults version="1.2">
<sample s="true" lb="ComputeIPAddr" rc="200" rm="OK" dt="text"/>
<httpSample s="true" lb="HTTP-Request-HC31" rc="200" rm="OK" dt="text">
<httpSample s="true" lb="HTTP-Request-HC31-0" rc="200" rm="OK" dt="text"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this test result is modified.
The result should be numbered sub results no ?

Copy link
Author

Choose a reason for hiding this comment

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

From the bug I understand, that JMeter will change subResults labels only if this labels are equals.
The parent label is HTTP-Request-HC31, subResults labels start with http://jmeter.apache.org/..... They are not equals and JMeter does not change them.

@@ -10,7 +10,7 @@
<queryString class="java.lang.String"></queryString>
<java.net.URL>file:testfiles/HTMLParserTestFile_2.html</java.net.URL>
</httpSample>
<httpSample s="true" lb="file:testfiles/HTMLParserTestFile_2.html-1" rc="200" rm="OK" tn="Thread Group 1-1" dt="text" de="" by="1321" sc="1" ec="0" ng="1" na="1">
Copy link
Contributor

Choose a reason for hiding this comment

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

Same note here

Copy link
Author

Choose a reason for hiding this comment

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

Same note here

@@ -10,7 +10,7 @@
<queryString class="java.lang.String"></queryString>
<java.net.URL>file:testfiles/HTMLParserTestFile_2.html</java.net.URL>
</httpSample>
<httpSample s="true" lb="file:testfiles/HTMLParserTestFile_2.html-1" rc="200" rm="OK" tn="Thread Group 1-1" dt="text" de="" by="1321" sc="1" ec="0" ng="1" na="1">
<httpSample s="true" lb="file:testfiles/HTMLParserTestFile_2_files/style.css" rc="200" rm="OK" tn="Thread Group 1-1" dt="text" de="" by="1321" sc="1" ec="0" ng="1" na="1">
Copy link
Contributor

Choose a reason for hiding this comment

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

Same note here

@@ -144,7 +144,7 @@
<httpSample s="true" lb="HR-FollowRedirect-18" rc="302" dt="text"/>
<httpSample s="true" lb="HR-FollowRedirect-19" rc="302" dt="text"/>
<httpSample s="true" lb="HR-FollowRedirect-20" rc="302" dt="text"/>
<httpSample s="false" lb="HR-FollowRedirect-21" rc="Non HTTP response code: java.io.IOException" dt="text"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this modification. Why 20 instead of 21 ?

Copy link
Author

Choose a reason for hiding this comment

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

Because first 20 subSamples were added here and the last SampleResult was generated from previous SubResult (that was renamed) and added here.

Copy link
Author

Choose a reason for hiding this comment

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

If you expect HR-FollowRedirect-21 here we can fix it using this small hack:

if (redirect >= MAX_REDIRECTS) {
            lastRes = errorResult(new IOException("Exceeded maximum number of redirects: " + MAX_REDIRECTS), new HTTPSampleResult(lastRes));
            lastRes.setSampleLabel(totalRes.getSampleLabel()); // use parent label 
            totalRes.addSubResult(lastRes);
}

@@ -120,7 +120,7 @@
<httpSample s="true" lb="HR-FollowRedirect-18" rc="302" dt="text"/>
<httpSample s="true" lb="HR-FollowRedirect-19" rc="302" dt="text"/>
<httpSample s="true" lb="HR-FollowRedirect-20" rc="302" dt="text"/>
<httpSample s="false" lb="HR-FollowRedirect-21" rc="Non HTTP response code: java.io.IOException" dt="text"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same note here

Copy link
Author

Choose a reason for hiding this comment

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

Same note here

@pmouawad
Copy link
Contributor

pmouawad commented Nov 9, 2018

Thanks Artem for the PR, but I don't understand it.
It seems to me it breaks what has been modified in 5.0.

@pmouawad
Copy link
Contributor

Hi @artem-fedorov,
Is it ok if we stay as we are ?
Reporter of issue says it’s ok for him.
Do you see any issue on your side that would need a fix ?

Thanks

@artem-fedorov
Copy link
Author

@pmouawad
In case when I have 3 resources for request url.com

  • url.com/res.css
  • url.com/res.html
  • url.com/res.xml
    Does current behaviour guaranteed that url.com/res.css always will have label url-0, url.com/res.html -> url.com-1 and url.com/res.xml -> url.com-2 ? and Labels will not be confused?

@pmouawad
Copy link
Contributor

pmouawad commented Nov 23, 2018

Hello @artem-fedorov ,
Yes if they are in the test plan.
No guarantee if they are automatically handled by resources download and it is done in parallel, but since 5.0, we add the URL in the CSV to disambiguate

@asfgit asfgit closed this in 7f4da0e Dec 21, 2018
@pmouawad
Copy link
Contributor

pmouawad commented Feb 4, 2019

Hello @artem-fedorov ,

There seem to be a place for this PR amended for users that use JMeter for functional testing as per the 2 last comments in:

Would you be ready to amend your PR to have old behaviour only if:

  • Test Plan has checked "Functional Test Mode"
  • There is a property "functional_mode=true"

Thanks

@artem-fedorov
Copy link
Author

Hello @pmouawad
I'll create a new PR soon

StorDm pushed a commit to etnetera/jmeter that referenced this pull request Jan 7, 2021
…losing issue.

This closes apache#429
Bugzilla Id: 62887

git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1849497 13f79535-47bb-0310-9956-ffa450edef68

Former-commit-id: 7f4da0e
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