[WIP] Introduce extension API for accessing arguments passed to tests #1614

paul-brooks · 2018-10-03T09:06:50Z

Overview

Addresses : #1139

Adding a BeforeParameterizedTestExecutionCallback to be called prior to test method execution with a copy of the actual test arguments.

Points for discussion:

Naming of the callback.
Currently, the arguments are resolved twice via the DefaultParameterResolver. It would be better to do this once (and pass a copy to the callback) but this would require a change to the ExecutableInvoker API. I'm happy to do this but thought I'd wait from feedback first.

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

codecov · 2018-10-03T09:16:19Z

Codecov Report

Merging #1614 into master will increase coverage by <.01%.
The diff coverage is 94.44%.

@@             Coverage Diff              @@
##             master    #1614      +/-   ##
============================================
+ Coverage     91.94%   91.94%   +<.01%     
- Complexity     3491     3496       +5     
============================================
  Files           321      322       +1     
  Lines          8329     8342      +13     
  Branches        722      723       +1     
============================================
+ Hits           7658     7670      +12     
- Misses          500      501       +1     
  Partials        171      171

Impacted Files	Coverage Δ	Complexity Δ
...r/engine/descriptor/TestFactoryTestDescriptor.java	`94.23% <100%> (-0.11%)`	`18 <0> (ø)`
...er/engine/descriptor/TestMethodTestDescriptor.java	`98.98% <100%> (+0.12%)`	`35 <3> (+3)`	⬆️
...it/jupiter/engine/execution/ExecutableInvoker.java	`76.92% <85.71%> (-13.25%)`	`6 <3> (-14)`
...t/jupiter/engine/execution/ParametersResolver.java	`94.11% <94.11%> (ø)`	`17 <17> (?)`
...jupiter/engine/execution/ExtensionValuesStore.java	`88.57% <0%> (-1.43%)`	`21% <0%> (-1%)`

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 a058aab...98c99e1. Read the comment docs.

sbrannen

Thanks for the PR.

Regarding the points you mentioned...

Naming of the callback.

We cannot call it "Parameterized", since the core extension API knows nothing about parameterized tests.

Such a callback should be generic for use with the invocation of any testable method.

Currently, the arguments are resolved twice via the DefaultParameterResolver. It would be better to do this once (and pass a copy to the callback) but this would require a change to the ExecutableInvoker API. I'm happy to do this but thought I'd wait from feedback first.

We most definitely cannot resolve parameters twice for a given invocation since doing so may cause undesired side effects in extensions that implement ParameterResolver.

Also, I'm not sure that we want to (or need to) extract such a DefaultParameterResolver from ExecutableInvoker.

So, please rework your PR accordingly.

Cheers,

Sam

sbrannen · 2018-10-03T10:59:04Z

Please see my notes in #1139 (comment) as well, especially the last paragraph.

paul-brooks · 2018-10-03T12:01:00Z

Thanks for the feedback Sam.

I figured as much about the naming. I'll just revert to ArgumentsProcessor.

Please see my notes in #1139 (comment) as well, especially the last paragraph.

I did miss that bit about the flag at the end of that comment, however wouldn't that mean that the ExecutableInvoker would need to know about and also invoke any ArgumentsProcessor's that are present (and also handle exceptions that they may throw)?

Isn't this what the TestMethodTestDescriptor already does for the existing callbacks?

It seems that we'd be adding to the responsibilities of the ExecutableInvoker with functionality that already exists in the TestMethodTestDescriptor.

Ultimately my thinking was that for test methods (which are only called from TestMethodTestDescriptor hierarchy?) we could resolve the parameters there and then pass them to a new method on ExecutableInvoker, thus only resolving them once and keeping the ExecutableInvoker clean.

Cheers

Paul

sbrannen · 2018-10-03T12:32:36Z

...piter-engine/src/main/java/org/junit/jupiter/engine/descriptor/TestMethodTestDescriptor.java

+	private void resolveParametersAndInvokeTestMethod(JupiterEngineExecutionContext context,
+			DynamicTestExecutor dynamicTestExecutor, ThrowableCollector throwableCollector) {
+		ExtensionRegistry registry = context.getExtensionRegistry();
+		Object[] arguments = parameterResolver.resolveParameters(getTestMethod(), Optional.empty(),


The test instance cannot be "empty". You'll have to retrieve that and pass it on like in invokeTestMethod().

sbrannen · 2018-10-03T12:34:53Z

...upiter-engine/src/main/java/org/junit/jupiter/engine/execution/DefaultParameterResolver.java

+import org.junit.platform.commons.logging.LoggerFactory;
+import org.junit.platform.commons.util.Preconditions;
+
+public class DefaultParameterResolver {


Since this not an implementation of ParameterResolver, we cannot call it DefaultParameterResolver.

Instead, we should name it something like ParametersResolver. The "s" is admittedly easy to overlook. So maybe we'll come up with something better later.

Not sure if you're agreeing to extract this as a class now?

Anyway, I've pushed some more changes with what I think you're saying.

Rename callback to ArgumentsProcessor. Rename DefaultParameterResolver to ParametersResolver. Now resolving test method parameters only once, and passing a copy to the ArgumentsProcessor. Test method is called using original resolved parameters via new method in ExecutableInvoker.

marcphilipp · 2019-07-19T10:51:31Z

Blocked by #1139 (comment).

stale · 2021-05-13T18:47:22Z

This pull request has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be closed if no further activity occurs. If you intend to work on this pull request, please reopen the PR. Thank you for your contributions.

stale · 2021-08-13T12:29:58Z

This pull request has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be closed if no further activity occurs. If you intend to work on this pull request, please reopen the PR. Thank you for your contributions.

stale · 2021-09-03T15:04:21Z

This pull request has been automatically closed due to inactivity. If you are still interested in contributing this, please ensure that it is rebased against the latest branch (usually main), all review comments have been addressed and the build is passing.

Adding a BeforeParameterizedTestExecutionCallback.

80140ec

paul-brooks changed the title ~~[WIP] Adding a BeforeParameterizedTestExecutionCallback.~~ [WIP] Introduce extension API for accessing arguments passed to tests Oct 3, 2018

sbrannen requested changes Oct 3, 2018

View reviewed changes

paul-brooks added 2 commits October 3, 2018 15:56

Adding javadoc & test for new ExecutableInvoker method

98c99e1

marcphilipp added the status: blocked label Jul 19, 2019

marcphilipp changed the base branch from master to main June 21, 2020 16:58

stale bot added the status: stale label May 13, 2021

stale bot removed the status: stale label May 26, 2021

stale bot added the status: stale label Aug 13, 2021

stale bot closed this Sep 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[WIP] Introduce extension API for accessing arguments passed to tests #1614

[WIP] Introduce extension API for accessing arguments passed to tests #1614

Uh oh!

paul-brooks commented Oct 3, 2018 •

edited

Loading

Uh oh!

codecov bot commented Oct 3, 2018 •

edited

Loading

Uh oh!

sbrannen left a comment

Uh oh!

sbrannen commented Oct 3, 2018

Uh oh!

paul-brooks commented Oct 3, 2018

Uh oh!

sbrannen Oct 3, 2018

Uh oh!

sbrannen Oct 3, 2018

Uh oh!

paul-brooks Oct 3, 2018

Uh oh!

marcphilipp commented Jul 19, 2019

Uh oh!

stale bot commented May 13, 2021

Uh oh!

stale bot commented Aug 13, 2021

Uh oh!

stale bot commented Sep 3, 2021

Uh oh!

Uh oh!

Uh oh!

[WIP] Introduce extension API for accessing arguments passed to tests #1614

[WIP] Introduce extension API for accessing arguments passed to tests #1614

Uh oh!

Conversation

paul-brooks commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Definition of Done

Uh oh!

codecov bot commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sbrannen left a comment

Choose a reason for hiding this comment

Uh oh!

sbrannen commented Oct 3, 2018

Uh oh!

paul-brooks commented Oct 3, 2018

Uh oh!

sbrannen Oct 3, 2018

Choose a reason for hiding this comment

Uh oh!

sbrannen Oct 3, 2018

Choose a reason for hiding this comment

Uh oh!

paul-brooks Oct 3, 2018

Choose a reason for hiding this comment

Uh oh!

marcphilipp commented Jul 19, 2019

Uh oh!

stale bot commented May 13, 2021

Uh oh!

stale bot commented Aug 13, 2021

Uh oh!

stale bot commented Sep 3, 2021

Uh oh!

Uh oh!

paul-brooks commented Oct 3, 2018 •

edited

Loading

codecov bot commented Oct 3, 2018 •

edited

Loading