Skip to content

[66139] Ensure Open Model Thread Group initializes config elements only once - #717

Merged
vlsi merged 6 commits into
apache:masterfrom
vlsi:omtg_configelement
May 4, 2023
Merged

[66139] Ensure Open Model Thread Group initializes config elements only once#717
vlsi merged 6 commits into
apache:masterfrom
vlsi:omtg_configelement

Conversation

@vlsi

@vlsi vlsi commented Jun 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@vlsi
vlsi force-pushed the omtg_configelement branch 5 times, most recently from 0a82bc2 to 5de2d3f Compare June 30, 2022 08:27
@codecov-commenter

codecov-commenter commented Jun 30, 2022

Copy link
Copy Markdown

Codecov Report

Merging #717 (0a82bc2) into master (7548000) will increase coverage by 0.08%.
The diff coverage is 34.61%.

❗ Current head 0a82bc2 differs from pull request most recent head 5de2d3f. Consider uploading reports for the commit 5de2d3f to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #717      +/-   ##
============================================
+ Coverage     55.23%   55.32%   +0.08%     
- Complexity    10381    10393      +12     
============================================
  Files          1062     1062              
  Lines         65760    65763       +3     
  Branches       7533     7534       +1     
============================================
+ Hits          36322    36381      +59     
+ Misses        26837    26777      -60     
- Partials       2601     2605       +4
Impacted Files Coverage Δ
...e/jmeter/threads/openmodel/OpenModelThreadGroup.kt 68.53% <0.00%> (+57.17%) ⬆️
.../apache/jmeter/threads/openmodel/scheduleParser.kt 68.93% <0.00%> (-0.68%) ⬇️
...ache/jmeter/threads/openmodel/scheduleTokenizer.kt 79.06% <0.00%> (-1.89%) ⬇️
...org/apache/jmeter/engine/StandardJMeterEngine.java 60.40% <36.36%> (+0.67%) ⬆️
...org/apache/jmeter/threads/AbstractThreadGroup.java 90.14% <50.00%> (ø)
...hreads/openmodel/OpenModelThreadGroupController.kt 100.00% <100.00%> (+75.00%) ⬆️
...jmeter/protocol/http/control/HttpMirrorServer.java 41.41% <0.00%> (-1.02%) ⬇️
...rg/apache/jmeter/threads/JMeterContextService.java 97.82% <0.00%> (+2.17%) ⬆️
...hreads/openmodel/ThreadScheduleProcessGenerator.kt 88.88% <0.00%> (+2.77%) ⬆️
...ache/jmeter/testelement/property/NullProperty.java 65.00% <0.00%> (+10.00%) ⬆️
... and 1 more

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 7548000...5de2d3f. Read the comment docs.

FSchumacher
FSchumacher approved these changes Jul 5, 2022
View reviewed changes

@FSchumacher FSchumacher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me, but note, that I am not really an Kotlin expert.

Comment thread ...src/test/kotlin/org/apache/jmeter/threads/openmodel/OpenModelThreadGroupConfigElementTest.kt
import org.junit.Test
import java.time.Duration

class OpenModelThreadGroupConfigElementTest : JMeterTestCase(), JMeterSerialTest {

@FSchumacher FSchumacher Jul 5, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Really nice test class.

Comment thread src/core/src/main/java/org/apache/jmeter/engine/StandardJMeterEngine.java
* Executor service to execute management tasks like "start test", "stop test".
* The use of {@link ExecutorService} allows propagating the exception from the threads.
*/
private static final ExecutorService EXECUTOR_SERVICE = Executors.newCachedThreadPool();

@FSchumacher FSchumacher Jul 5, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Have you considered using an Thread.UncaughtExceptionHandler instead?

@vlsi vlsi Jul 5, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I do not think UncaughtExceptionHandler makes things any easier/better. I think EXECUTOR_SERVICE.submit(...) gives exception tracking for free, and we should move away from new Thread anyway.

The main problem here is that StandardJMeterEngine.runTest returns void, and there's no way for the client to detect if the test failed or not.

In the ideal case, StandardJMeterEngine.runTest should return something like Future<...>, so the client could await termination or cancel it, or get the results.

However, the change of runTest to return Future is out of the scope of this PR. In this PR, I added the ability to execute and wait for results from StandardJMeterEngine from unit-test code. So I did the minimal set of changes only.

@vlsi

vlsi commented Jul 7, 2022

Copy link
Copy Markdown
Collaborator Author

The test keeps failing at Travis.
I'm inclined to disable the test and commit the code.

@vlsi
vlsi force-pushed the omtg_configelement branch 2 times, most recently from 9148f9d to 1324f6b Compare July 8, 2022 14:05
@vlsi
vlsi force-pushed the omtg_configelement branch from 1324f6b to 88fa709 Compare May 4, 2023 10:28
@vlsi
vlsi force-pushed the omtg_configelement branch from 88fa709 to b069225 Compare May 4, 2023 10:33
@vlsi
vlsi merged commit b069225 into apache:master May 4, 2023
@vlsi vlsi mentioned this pull request May 10, 2023
Closed
vlsi added a commit that referenced this pull request May 11, 2023
@vlsi
fix: reduce delay for stopping JMeter in non-GUI mode from 60 to 1 se…
c083b2a 
…cond

#717 introduced cachedThreadPool
in StandardJMeterEngine, and it had 60 seconds inactivity timeout by default.
It caused non-GUI tests to wait for the threads to cleanup.
@vlsi vlsi added this to the 5.6 milestone May 11, 2023
@vlsi
vlsi deleted the omtg_configelement branch June 5, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FSchumacher FSchumacher FSchumacher approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

5.6

Development

Successfully merging this pull request may close these issues.

3 participants

@vlsi @codecov-commenter @FSchumacher