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

Disable Parameterised tests selectively #163

Closed
nishant-vashisth opened this issue Jan 21, 2020 · 13 comments
Closed

Disable Parameterised tests selectively #163

nishant-vashisth opened this issue Jan 21, 2020 · 13 comments

Comments

@nishant-vashisth
Copy link
Contributor

nishant-vashisth commented Jan 21, 2020

Current situation

Currently any @disabled extensions that are there in the junit5 framework disable any test at the class level or at the invocation level before any of the parameters are resolved and the tests are launched

This leads to disable all the tests that were potentially going to run from any of the Parameterized test sources and it reports the test as only one skipped test.

Suggestion

To possible have an extension similar to @disabled (and it's counterparts) which is able to disable a single instance of the Parameterized test rather than all of them and only marks that individual test as skipped while others can run

Currently because of the following it is not straight forward to build something like this
junit-team/junit5#1139
junit-team/junit5#1668
junit-team/junit5#1884

And the only way is to use reflection to get access to the arguments for a Parameterized test's resolved argument

It would be nice to have native support for this as well, however as I understand, for programatic argument sources, specially ArgumentProvider and MethodSource, it isn't always sensible to let the arguments get initialised if we're skipping the tests

Currently I've built the following solution for this (it needs to be "productionalized" the answer is just a hint to actual impl)
https://stackoverflow.com/questions/59716009/is-it-possible-to-disable-a-parameterized-test-basis-arguments-in-junit5

However, it's only valid till Junit5 version 5.3.x, after 5.5.x the Extension classes are package-private and the same solution needs a little bit of tweaking.

@nipafx nipafx added 🏗️ type: enhancement 🚦 status: in discussion ⚙️ component: Jupiter Issues coming from (internal/official) Jupiter features etc. labels Jan 21, 2020
@nipafx
Copy link
Member

nipafx commented Jan 21, 2020

That's a very interesting idea @nishantvas. I looked at the issues and thought about the use case. I don't think I'd like a reflection-based solution because it is too brittle, specifically across Jupiter versions (I try to avoid specific Pioneer versions only working for specific Jupiter versions). I played a little with argument converters, but they're not good fit for this either.

But why so complicated? What about a simple declarative API?

@ParameterizedTest
@CsvSource({"1, a", "1, b", "2, a", "2, b"})
void parameterizedTest(int number, String string) {
	Disable
		.when(number, string)
		.is(2, "a")
		.is(2, "b");

	// actual test code
	if (number % 2 == 0)
		throw new IllegalArgumentException();
}

private static class Disable {

	private final Object[] arguments;

	private Disable(Object[] arguments) {
		this.arguments = arguments;
	}

	public static Disable when(Object... arguments) {
		return new Disable(arguments);
	}

	public Disable is(Object... values) {
		if (Arrays.equals(arguments, values))
			throw new TestAbortedException("Test aborted for arguments " + Arrays.toString(values));
		return this;
	}

}

Let me know what you think.

@nishant-vashisth
Copy link
Contributor Author

nishant-vashisth commented Jan 22, 2020

You're right, this does result in the same effect. For someone looking to build a straight forward solution this works perfectly.

Although I personally don't prefer programatic solutions for such things which need to be put in a test body. I incline more towards meta data like annotations because they're more readable and indicate their purpose better. Also, I have some extra fields in the disable annotation in my particular case which can't be processed from the test itself.
I have to extract the arguments for other purposes so the reflection answer I wrote was a bit more natural for me to write.

Thanks for coming up with this immensely simple solution.

I don't think there is a need for or possibility of creating any extension which can achieve the same effect without over engineering things.
Up to you to close the issue :)

@nipafx
Copy link
Member

nipafx commented Jan 23, 2020

Although I personally don't prefer programatic solutions for such things which need to be put in a test body. I incline more towards meta data like annotations because they're more readable and indicate their purpose better.

You're right about that, I usually prefer such a solution for those exact reasons, but since it seems to come at quite a cost here, I think the programmatic approach can be ok.

Also, I have some extra fields in the disable annotation in my particular case which can't be processed from the test itself.

I'd be curious to learn more about that to better understand the general use case. 😊

Up to you to close the issue :)

I kinda like the idea, so I'm still thinking about implementing it. 😁

@nishant-vashisth
Copy link
Contributor Author

nishant-vashisth commented Jan 23, 2020

I'd be curious to learn more about that to better understand the general use case. 😊

I'm using this with allure and am using the annotations to process tests which are disabled to link to their individual issue tags.

Since each failure in the parameterized tests can be due to a different bug, using an annotation let's me mark it better lest I take up the task of updating the test code to save this data in some common place. Which is never a good idea.

I kinda like the idea, so I'm still thinking about implementing it. 😁

Let me know how you think this should be implemented as an extension and I can work on building it.

@nipafx
Copy link
Member

nipafx commented Feb 11, 2020

While I agree with your general preference to use an annotation-based approach, I still don't quite get whether it's actually necessary or not. If you want to link to an issue for disabled argument combinations, we could do something like:

@ParameterizedTest
@CsvSource({"1, a", "1, b", "2, a", "2, b"})
void parameterizedTest(int number, String string) {
	Disable
		.when(number, string)
		.is(2, "a")
		.becauseOf("https://our.jira.company.com/issues/PROJ-123")
	Disable
		.when(number, string)
		.is(2, "b")
		.becauseOf("https://our.jira.company.com/issues/PROJ-125")

	// actual test code
	if (number % 2 == 0)
		throw new IllegalArgumentException();
}

Anyway, I convinced myself that this is a valid feature request, so if anybody wants to implement it, leave a comment here and go for it!

@Michael1993
Copy link
Member

I'd like to give this a try.

@Michael1993
Copy link
Member

I do have a related question, though. Say, you disable the tests and your build runs fine. How will you remember to enable the tests later? Wouldn't it be better to create a separate test for the specific test case and annotate it with @DisableUntil(/*planned date of fix*/)?

@nishant-vashisth
Copy link
Contributor Author

nishant-vashisth commented Feb 18, 2020

You can always mark them as @Disabled("Planned fix DD-MM-YYYY")
Since even if they start working after the fix date, I imagine you would want to go back and remove the now unused DisableUntil annotation.

Alternatively, like many people do, you can mark the fixing issue on the @Disabled annotation, and with each fixed issue, go find it's occurences and run them.

What I'm trying to say is, you're going to have to go back to those @DisableUntil to clean them anyhow, so you can choose many other ways to mark the same tests making the easy to find. Also, if the tests don't get fixed till then, you're now stuck with updating the annotation to fix your failing builds

@nishant-vashisth
Copy link
Contributor Author

@nicolaiparlog, so my idea with disabling parameterized tests was to support allure reporting.

Namely, I could use allure meta annotations to point to individual failing tests and to enable me to assign an Issue property to the parameterized tests.

Creating as an extension would allow people to meta annotate the extension and achieve something similar.

@nipafx
Copy link
Member

nipafx commented Feb 18, 2020

Let me see if got this right, @nishantvas. Say we would create an annotation that works as we want, e.g.:

@DisableParamterizedTest("foo", 5) // in JUnit Pioneer
void test(String s, int i) {
	...
}

Then you would like to combine it with allure reporting (which I know nothing about, so I may totally misunderstand you) to create your own annotation:

// declaration
@AllureReportingAnnotation
@DisableParamterizedTest
public @interface MyOwnDisableParamterizedTest { }

// use
@MyOwnDisableParamterizedTest("foo", 5)
void test(String s, int i) {
	...
}

If that's what you're after, I have bad news: There's no way for my extension to get to "foo" and 5 - @DisableParamterizedTest would just not be usable as a meta-annotation.

@nishant-vashisth
Copy link
Contributor Author

nishant-vashisth commented Feb 19, 2020

@nicolaiparlog so It's almost how you're putting it, however, as I mentioned when I raised this

Currently because of the following it is not straight forward to build something like this
junit-team/junit5#1139
junit-team/junit5#1668
junit-team/junit5#1884
And the only way is to use reflection to get access to the arguments for a Parameterized test's resolved argument

The Extensions could very easily have access to "foo" and 5 if the ExtensionContext api has access to Arguments, unfortunately it doesn't.

Since we don't want to go with the option of using reflection to access the Arguments till the above issues get fixed, we can however, use the test name to provide something which can do this.

@DisableParamterizedTest(whenName = "foo", regex = true , contains = true) // either regex or contains or both
@ParameterizedTest(name = "Test {0} as {1}")
void test(String s, int i) {
	...
}

And them the AllureSetup was right on point, it uses the some Meta annotations similar to how you mentioned to add the value to Allure reports.

@LinkAnnotation(type = ISSUE_LINK_TYPE) // Allure Meta
@Repeatable(DisableParamterizedTests.class) // So we can have individual disabling setup
@ExtendWith(DisableParameterExtension.class)
public @interface DisableParamterizedTest {
.
.
.
}

One thing to note would be that we want to disable individual parameterized tests, therefore, we will have to let the ParameterResolvers to run and in the extension we only run it when it's at test level.

public class DisableParameterExtension extends ExecutionConditionEvaluator {

  @Override
  public ConditionEvaluationResult evaluateExecutionCondition(ExtensionContext context) {
    if (context.getTestMethod().isPresent()) {
      .
      .
      .
    }
  }
}

@nishant-vashisth
Copy link
Contributor Author

nishant-vashisth commented Feb 19, 2020

I pushed an example PR to show what I meant.
#175

It is achieving the same effect as the snippet you mentioned,

Disable
		.when(number, string)
		.is(2, "a")
		.becauseOf("https://our.jira.company.com/issues/PROJ-123")

However, by having this as an annotation, I can fetch this in my reports and it's more accessible to other extensions or listeners if required (like in my case with Allure)

@nipafx
Copy link
Member

nipafx commented Jun 20, 2020

We just updated to Jupiter 5.5 (#278) to use the new InvocationInterceptor API. If that API gets called for individual parameterized test invocations, it would be a very powerful way to implement disabling parameterized tests. The provided Invocation allows skipping tests and the ReflectiveInvocationContext seems to give access to the actual arguments. Someone should look into that and possibly create an issue if the approach seems promising. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants