Add timeout config parameter #2026

chrischild · 2019-09-24T19:34:17Z

Overview

Added a new configuration parameter (junit.jupiter.execution.timeout.mode) to determine if timeouts should be applied to annotated tests.

Couple of notes/questions:

Since the default was to have it be enabled but still allow someone to comment the parameter out I wondered whether there was any reason to see if the value was set to "enabled" or not. To me there's no reason so I only checked to see if the value was "disabled" or "disabled_on_debug". Thoughts?
Testing: I created one new test to make sure that setting the value to "disabled" truly did disable the timeouts. If there's a way to test the "disabled_on_debug" let me know.

Resolves #1959.

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

Definition of Done

There are no TODOs left in the code
Method preconditions are checked and documented in the method's Javadoc
Coding conventions (e.g. for logging) have been followed
Change is covered by automated tests including corner cases, errors, and exception handling
Public API has Javadoc and @API annotations
Change is documented in the User Guide and Release Notes
All continuous integration builds pass

sormuras

Didn't look into the implementation, yet -- but already found some issues.

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TimeoutExtension.java

...t-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/TimeoutExtensionTests.java

chrischild · 2019-09-24T20:35:48Z

I'm obviously hitting some errors but when I run gradlew test locally I'm seeing everything pass. What should I be running to see these errors?

sormuras · 2019-09-24T20:39:56Z

junit5/.github/workflows/main.yml

Line 24 in da135a4

    
                   ./gradlew --scan --warning-mode=all -Dplatform.tooling.support.tests.enabled=true build

Without the '--scan' option, if you don't want to.

chrischild · 2019-09-24T21:56:31Z

Closing for now. Thought I was closer to done but I looks like that was incorrect.

junit-jupiter-engine/src/module/org.junit.jupiter.engine/module-info.java

marcphilipp

Thanks for the PR!

junit-jupiter-engine/src/module/org.junit.jupiter.engine/module-info.java

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TimeoutExtension.java

sormuras · 2019-09-30T06:54:55Z

ManagementFactory.getRuntimeMXBean().getInputArguments()
...
Checking for the -agentlib:jdwp should suffice.
...

Parsing the input arguments feels a brittle to me. Isn't there another and safer way to determine whether a VM is running in debug mode at run time? /cc @raphw @forax

raphw · 2019-10-02T13:24:18Z

ManagementFactory.getRuntimeMXBean().getInputArguments()
...
Checking for the -agentlib:jdwp should suffice.
...

Parsing the input arguments feels a brittle to me. Isn't there another and safer way to determine whether a VM is running in debug mode at run time? /cc @raphw @forax

Not really, you can check if the VM is listening to port 5005 but this does not help if the debugging port has changed. As far as I know, there is no good mechanism.

sbrannen · 2019-10-02T13:42:21Z

Not really, you can check if the VM is listening to port 5005 but this does not help if the debugging port has changed. As far as I know, there is no good mechanism.

Thanks for the feedback, @raphw.

So checking only for -agentlib:jdwp seems to still be the best option.

sormuras · 2019-10-08T10:06:57Z

junit-platform-commons/src/module/org.junit.platform.commons/module-info.java

@@ -9,7 +9,8 @@
 */

 module org.junit.platform.commons {
-	requires java.logging; // TODO Is "requires transitive java.logging" needed here?


Removed the superseded TODO on-the-fly -- the module API of org.junit.platform.commons does not use types of java.logging.

sormuras · 2019-10-08T10:08:34Z

junit-platform-commons/src/module/org.junit.platform.commons/module-info.java

@@ -9,7 +9,8 @@
 */

 module org.junit.platform.commons {
-	requires java.logging; // TODO Is "requires transitive java.logging" needed here?
+	requires java.logging;


I left the static modifier out on purpose -- because we need the module when running on the module path.

Otherwise, users have to add it via --add-module java.management or via their module descriptor(s).

sormuras · 2019-10-08T10:14:15Z

Since the default was to have it be enabled but still allow someone to comment the parameter out I wondered whether there was any reason to see if the value was set to "enabled" or not. To me there's no reason so I only checked to see if the value was "disabled" or "disabled_on_debug". Thoughts?

With ce1892c I enforced enabled to be the actual value for when a mode value is passed.

Testing: I created one new test to make sure that setting the value to "disabled" truly did disable the timeouts. If there's a way to test the "disabled_on_debug" let me know.

That "disabled_on_debug" test case and the one that simulates an environment without JMX is still missing.

Superseded by ce1892c

chrischild · 2019-10-08T15:03:30Z

Ah I like it. Ok I'll take a peek and try and get those final missing pieces finished soon.

chrischild · 2019-10-09T01:50:44Z

Huh I'm stumped on the JMX environment test. I tried my tests that I had created earlier and there was never a failure. I then commented out the java.management module change made by @sormuras in the platform module-info file

        requires java.logging;
//	requires java.management; // needed by RuntimeUtils to determine input arguments
	requires transitive org.apiguardian.api;

Also removed it from junit-platform-commons.expected to make sure a build should pass. I cleaned and then rebuilt and no errors occurred during the build.

I rechecked out this branch fresh and again removed the java.management line above and re-tested a clean build a third time and again there was no errors thrown. I don't see how this is possible.

Anyways I'll check in the new unit test for the disabled_on_debug value.

...t-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/TimeoutExtensionTests.java

sormuras · 2019-10-09T06:44:16Z

Huh I'm stumped on the JMX environment test. [...]

I'll add it later this week to this branch ... or after merging it. Or #2054 will provide such an integration test as a side effect.

marcphilipp · 2019-10-10T17:40:06Z

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/Constants.java

+	 * </ul>
+	 *
+	 * @since 5.6
+	 */


The Javadoc should document the default value.

marcphilipp · 2019-10-10T17:40:36Z

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TimeoutExtension.java

@@ -38,6 +42,9 @@
 	private static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(Timeout.class);
 	private static final String TESTABLE_METHOD_TIMEOUT_KEY = "testable_method_timeout_from_annotation";
 	private static final String GLOBAL_TIMEOUT_CONFIG_KEY = "global_timeout_config";
+	public static final String ENABLED_MODE_VALUE = "enabled";
+	public static final String DISABLED_MODE_VALUE = "disabled";
+	public static final String DISABLED_ON_DEBUG_MODE_VALUE = "disabled_on_debug";


These should be private.

sormuras · 2019-10-11T05:04:06Z

Thanks @chrischild

chrischild · 2019-10-11T05:29:17Z

My pleasure. I wish I could've figured out the environment test though. I went back again and tried to see what else I might have missed but couldn't find a solution. I'll be keeping an eye on the Android integration test so I can see how it gets solved.

Added timeout config parameter

b33dbc4

sormuras previously requested changes Sep 24, 2019

View reviewed changes

Chris added 3 commits September 24, 2019 12:50

Fixed checkstyle error

5fc99d7

fixed requested changes

7b85300

Fixed JarDescribeModuleTests error

5162dd3

chrischild closed this Sep 24, 2019

Fixed build errors

29852d8

chrischild reopened this Sep 24, 2019

chrischild requested a review from sormuras September 24, 2019 23:08

Modified Timeout Constant

dec9f71

marcphilipp self-requested a review September 25, 2019 07:57

sormuras reviewed Sep 25, 2019

View reviewed changes

junit-jupiter-engine/src/module/org.junit.jupiter.engine/module-info.java Outdated Show resolved Hide resolved

Fixed requested changes

43a672e

marcphilipp added this to the 5.6 M1 milestone Sep 27, 2019

marcphilipp requested changes Sep 27, 2019

View reviewed changes

junit-jupiter-engine/src/module/org.junit.jupiter.engine/module-info.java Outdated Show resolved Hide resolved

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TimeoutExtension.java Outdated Show resolved Hide resolved

sbrannen reviewed Sep 27, 2019

View reviewed changes

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TimeoutExtension.java Outdated Show resolved Hide resolved

Requested Changes without Integration Test

1ba3bda

marcphilipp assigned sormuras Oct 3, 2019

Move runtime input arguments getter to dedicated utility class

ce1892c

sormuras reviewed Oct 8, 2019

View reviewed changes

sormuras requested a review from marcphilipp October 8, 2019 10:16

Created new test for Timeout config (1959)

8eb8211

chrischild commented Oct 9, 2019

View reviewed changes

...t-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/TimeoutExtensionTests.java Show resolved Hide resolved

sormuras approved these changes Oct 10, 2019

View reviewed changes

marcphilipp approved these changes Oct 10, 2019

View reviewed changes

sormuras changed the title ~~Added timeout config parameter~~ Add timeout config parameter Oct 11, 2019

sormuras merged commit ddb8c61 into junit-team:master Oct 11, 2019

sormuras added component: Jupiter theme: extensions type: enhancement labels Oct 11, 2019

sormuras mentioned this pull request Mar 27, 2020

Add Android integration test project junit-team/junit5-samples#125

Open

2 tasks

sbrannen mentioned this pull request Jul 9, 2022

Improve debug mode detection in JUnit Jupiter #2973

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add timeout config parameter #2026

Add timeout config parameter #2026

chrischild commented Sep 24, 2019 •

edited by sormuras

Loading

sormuras left a comment

chrischild commented Sep 24, 2019

sormuras commented Sep 24, 2019

chrischild commented Sep 24, 2019

marcphilipp left a comment

sormuras commented Sep 30, 2019 •

edited

Loading

raphw commented Oct 2, 2019

sbrannen commented Oct 2, 2019

sormuras Oct 8, 2019

sormuras Oct 8, 2019

sormuras commented Oct 8, 2019

chrischild commented Oct 8, 2019

chrischild commented Oct 9, 2019 •

edited

Loading

sormuras commented Oct 9, 2019

marcphilipp Oct 10, 2019

marcphilipp Oct 10, 2019

sormuras commented Oct 11, 2019

chrischild commented Oct 11, 2019

Add timeout config parameter #2026

Add timeout config parameter #2026

Conversation

chrischild commented Sep 24, 2019 • edited by sormuras Loading

Overview

Definition of Done

sormuras left a comment

Choose a reason for hiding this comment

chrischild commented Sep 24, 2019

sormuras commented Sep 24, 2019

chrischild commented Sep 24, 2019

marcphilipp left a comment

Choose a reason for hiding this comment

sormuras commented Sep 30, 2019 • edited Loading

raphw commented Oct 2, 2019

sbrannen commented Oct 2, 2019

sormuras Oct 8, 2019

Choose a reason for hiding this comment

sormuras Oct 8, 2019

Choose a reason for hiding this comment

sormuras commented Oct 8, 2019

chrischild commented Oct 8, 2019

chrischild commented Oct 9, 2019 • edited Loading

sormuras commented Oct 9, 2019

marcphilipp Oct 10, 2019

Choose a reason for hiding this comment

marcphilipp Oct 10, 2019

Choose a reason for hiding this comment

sormuras commented Oct 11, 2019

chrischild commented Oct 11, 2019

chrischild commented Sep 24, 2019 •

edited by sormuras

Loading

sormuras commented Sep 30, 2019 •

edited

Loading

chrischild commented Oct 9, 2019 •

edited

Loading