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

improve junit vintage docs about parallel execution #4336

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

etrandafir93
Copy link

Issue: #4321

Overview

Improving user guide section about parallel test execution at class/methods level with JUnit Vintage.


I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

junit.vintage.execution.parallel.methods=true
junit.vintage.execution.parallel.pool-size=4
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this?

Enable/disable parallel execution (defaults to `false`). Requires opt-in for `classes`
or `methods` to be executed in parallel using the configuration parameters below.
Enable/disable parallel execution (defaults to `false`).
Requires opt-in for `classes` or `methods` to be executed in parallel using the configuration parameters below.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Requires opt-in for `classes` or `methods` to be executed in parallel using the configuration parameters below.
Requires opt-in for `classes` or `methods` to be executed in parallel using the configuration parameters below.

Specifies the size of the thread pool to be used for parallel execution. By default, the
number of available processors is used.
Specifies the size of the thread pool to be used for parallel execution.
By default, the number of available processors is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
By default, the number of available processors is used.
By default, the number of available processors is used.

Copy link
Author

Choose a reason for hiding this comment

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

I added the spaces, as suggested - but do we need them?

I have added the brake lines to comply with this sembr convention, but I won't have an effect on the way the text is displayed. That's why I haven't added any space at the start of the row.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines 67 to 68
junit.vintage.execution.parallel.enabled=true
junit.vintage.execution.parallel.classes=true
Copy link
Contributor

@YongGoose YongGoose Feb 24, 2025

Choose a reason for hiding this comment

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

With this setup, the VintageTestEngine will use two different threads

Suggested change
junit.vintage.execution.parallel.enabled=true
junit.vintage.execution.parallel.classes=true
junit.vintage.execution.parallel.enabled=true
junit.vintage.execution.parallel.classes=true
junit.vintage.execution.parallel.pool-size=2

Copy link
Author

@etrandafir93 etrandafir93 Feb 26, 2025

Choose a reason for hiding this comment

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

I left this property out to allow the reader to focus on the ones that directly impact the example.

For the given example with 2 test classes and parallelization enabled per class, we'll always use 2 threads - even if the thread pool size is larger, correct?

Nonetheless, I have added it back now (and to the other examples as well).

Copy link
Contributor

@YongGoose YongGoose Feb 26, 2025

Choose a reason for hiding this comment

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

For the given example with 2 test classes and parallelization enabled per class, we'll always use 2 threads - even if the thread pool size is larger, correct?

That’s correct.

While you could explain it by saying that running FooTest and BarTest uses two threads, I believe it is more appropriate to relate it to the size of the thread pool rather than directly linking the number of tests to the number of threads. For example, when running three tests in parallel with a thread pool size of two, only two threads will be used, regardless of the number of tests.

What do you think? @marcphilipp

Comment on lines +111 to +113
junit.vintage.execution.parallel.enabled=true
junit.vintage.execution.parallel.classes=true
junit.vintage.execution.parallel.methods=true
Copy link
Contributor

Choose a reason for hiding this comment

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

The default number of threads is Runtime.getRuntime().availableProcessors() unless specified otherwise.
So, it would be a good idea to include a configuration setting to define the number of threads.

Copy link
Contributor

@YongGoose YongGoose left a comment

Choose a reason for hiding this comment

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

It would be great to add documentation for the following cases as well.

The document looks much better now.
Thank you! 🙂

junit.vintage.execution.parallel.enabled=false
junit.vintage.execution.parallel.classes=true
junit.vintage.execution.parallel.methods=true

@etrandafir93
Copy link
Author

junit.vintage.execution.parallel.enabled=false
junit.vintage.execution.parallel.classes=true
junit.vintage.execution.parallel.methods=true

@YongGoose - thanks for the feedback. I have updated the PR now addressing the comments.
I have broken down the content into 4 short subsections, as the main section grew.

I have added the example above as well as the case where parallel execution is enabled but no parallel execution level is defined - to highlight this misconfiguration as well.

Let me know what you think:)

@etrandafir93 etrandafir93 force-pushed the issues/4321-improve-vintages-docs branch from 0efba29 to c6831bb Compare February 26, 2025 07:23
Enable/disable parallel execution (defaults to `false`). Requires opt-in for `classes`
or `methods` to be executed in parallel using the configuration parameters below.
Enable/disable parallel execution (defaults to `false`).
Requires opt-in for `classes` or `methods` to be executed in parallel using the configuration parameters below.
Copy link
Contributor

@YongGoose YongGoose Feb 26, 2025

Choose a reason for hiding this comment

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

There should be two spaces at the start :)

I’ve also had more feedback on documentation than on code when contributing. 😂

Copy link
Author

Choose a reason for hiding this comment

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

oh 😅 fixed now. I wonder if spotless can be configured to catch this

Copy link
Contributor

@YongGoose YongGoose Feb 26, 2025

Choose a reason for hiding this comment

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

I’m not very familiar with Spotless, so I’m not sure if it provides that feature. 😁
If it does, it might be a good idea to discuss it with Marc and consider contributing.
It looks like the file below is used for Spotless configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

As a former contributor to Spotless, there's nothing technical stopping us from adopting such a check. It would "just" require implementing a regex (which the Gradle plugin has native support for) or finding a JVM library or CLI tool that does the job.

However, as English uses full stops for ends of sentences, word shortenings (for example, "regarding" to "re.") and inside other words sometimes (for example, "01.01.2025"), this may be tricky to do without heuristics unless we're happy to catch all full stops followed one space or 3+ spaces.

@etrandafir93 etrandafir93 force-pushed the issues/4321-improve-vintages-docs branch from c6831bb to 3432f69 Compare February 26, 2025 07:31
@YongGoose
Copy link
Contributor

YongGoose commented Feb 26, 2025

@YongGoose - thanks for the feedback. I have updated the PR now addressing the comments.
I have broken down the content into 4 short subsections, as the main section grew.

I have added the example above as well as the case where parallel execution is enabled but no parallel execution level is defined - to highlight this misconfiguration as well.

Let me know what you think:)

LGTM 👍

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.

3 participants