Add extension to disable per test name to be able to disable parameterized tests #175

nishant-vashisth · 2020-02-19T14:20:58Z

Additional extension to be able to skip tests based on test name
This is particularly helpful when dealing with Parameterized or Dynamically Registered tests

#163

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

…rized tests

nipafx · 2020-03-03T21:49:46Z

Sorry @nishantvas, I didn't have time to look at this and I'm pretty busy the next weeks. I will get to it eventually, I promise! :)

nishant-vashisth · 2020-03-03T22:30:13Z

I didn't really add this to be merged (yet) it was more to indicate the Idea I was trying to indicate with my Issue

nipafx · 2020-03-26T20:47:19Z

Hey @nishantvas, this is a pretty cool proof-of-concept! I wasn't aware that the conditional extension is executed for each parameterized test run - very clever to make use of that and the test display name. 👍 Keep working on it, it is definitely going into the right direction.

nipafx

Just a quick comment. Let me know when you fleshed it out and are interested in a detailed review.

src/main/java/org/junitpioneer/jupiter/DisableIfNameExtension.java

nipafx · 2020-03-26T20:51:17Z

Of course we still need examples, documentation, and commit message. In case you prefer only writing the code, that's ok as well, just let me know.

nipafx · 2020-03-26T20:53:52Z

One more note, should it be @DisableIfDisplayName?

Bukama · 2020-03-26T20:55:51Z

One more note, should it be @DisableIfDisplayName?

I vote for this so it's clear by the annotation name that the display name is taken into account and not the "coded name"

Bukama · 2020-03-26T20:56:22Z

Of course we still need examples, documentation, and commit message. In case you prefer only writing the code, that's ok as well, just let me know.

Want to add Javadoc at least for the annotation interface

nishant-vashisth · 2020-03-26T21:48:54Z

One more note, should it be @DisableIfDisplayName?

Sure, makes sense and is more readable.

I can also work on finalising this(the code is more of less in it's final form) and add more examples, documentations.

How do you mean commit messages? Should I renamed the current commit messages? Is there any standard format?

… into parameterized

Bukama · 2020-03-27T18:13:01Z

One more note, should it be @DisableIfDisplayName?

Sure, makes sense and is more readable.

I can also work on finalising this(the code is more of less in it's final form) and add more examples, documentations.

That would be great!

How do you mean commit messages? Should I renamed the current commit messages? Is there any standard format?

Only the text for the final commit is needed as all the previous commits will get squashed while merging the branch into master - most probably done by @nicolaiparlog when you call it finished.

nipafx · 2020-03-29T10:30:22Z

I can also work on finalising this(the code is more of less in it's final form) and add more examples, documentations.

Great, let me know when you're done and I'll review the PR then.

How do you mean commit messages? Should I renamed the current commit messages? Is there any standard format?

As Bukama mentioned, all commits will be squashed into one and then applied to master. That commit's message has a standard format, yes. You can propose a message if you want or I can write it if you prefer - your choice. :)

nishant-vashisth · 2020-03-30T18:07:21Z

I've added documentation and some more examples. Also updated the extension's name

It made more sense to be able to have control to disable multiple tests in a set of Parameterized tests.
However, when I tried to make it a repeatable annotation, the Extension seem to get registered multiple times and was posing problems.

Unfortunately, I'll be away for a while (PC access) @nicolaiparlog , @Bukama If there is something amiss, it's perfectly fine if you guys want to pick that up.. Otherwise, i can take care of it next week or so.

… into parameterized

Bukama

Some minior improvement suggestions. Please take a look at them :)

Bukama · 2020-04-01T18:21:39Z

docs/disable-if-display-name.adoc

@@ -0,0 +1,42 @@
+:page-title: @DisableIfDisplayName
+:page-description: JUnit Jupiter extensions to selectively disable Parameterized tests


(multiple) Parameterized ==> parameterized

Bukama · 2020-04-01T18:23:13Z

docs/disable-if-display-name.adoc

+:page-title: @DisableIfDisplayName
+:page-description: JUnit Jupiter extensions to selectively disable Parameterized tests
+
+The `@DisableIfDisplayName` annotation Junit Jupiter extension that can be selectively used to disable Parameterized tests which are dynamically registered on runtime basis their display name.


This reads wired. What about this:

The @DisableIfDisplayName annotation can be used to selectivevly disable parameterized....

Bukama · 2020-04-01T18:23:54Z

docs/disable-if-display-name.adoc

+The `@DisableIfDisplayName` annotation Junit Jupiter extension that can be selectively used to disable Parameterized tests which are dynamically registered on runtime basis their display name.
+The annotations is only supported on test method level for Parameterized tests.
+Unlike the `@Disabled` API provided in Junit which disables the test on first encounter of the annotation
+`@DisableIfDisplayName` is validated before each `@ParameterizedTest` so that each test (name) can be individually evaluated


Missing . at the end of the sentence.

Bukama · 2020-04-01T18:25:12Z

docs/docs-nav.yml

@@ -16,3 +16,5 @@
        url: /docs/temp-directory/
      - title: "Vintage @Test"
        url: /docs/vintage-test/
+      - title: "@DisableIfDisplayName"


Please move up so it's sorted in alphabetically order

Bukama · 2020-04-01T18:25:58Z

src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

+
+/**
+ * {@code @DisableIfDisplayName} is a JUnit Jupiter extension which can be used to
+ * selectively disable {@link ParameterizedTest} basis their {@link ExtensionContext#getDisplayName()}


basis their

Do you mean based on their ?

Bukama · 2020-04-01T18:28:12Z

src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

+ * If it is required that we wish to disable selective tests out of the plethora of dynamically
+ * registered Parameterized tests, then we can utilize the following
+ *
+ * Each repeatable annotation will be processed for each test, and Test will be skipped if


and the test will be

Bukama · 2020-04-01T18:28:57Z

src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

+ * registered Parameterized tests, then we can utilize the following
+ *
+ * Each repeatable annotation will be processed for each test, and Test will be skipped if
+ * any of them evaluate to be true against the display name


evaluates true against the display name.

Bukama · 2020-04-01T18:35:54Z

src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

+@ExtendWith(DisableIfNameExtension.class)
+public @interface DisableIfDisplayName {
+
+	/**


Displaynames of the tests cases to be disabled. The whole test case name can be stored as well as sub strings of it. The values will be evaluated [...] by default. If {code regex} is provided, the string will be evaluated as ... display name. @return Test case display name

Bukama · 2020-04-01T18:37:53Z

src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

+	/**
+	 * @return if the {@code value} is to be evaluated as regular expression or sub-string
+	 */
+	boolean regex() default false;


Suggest to rename it to isRegEx. The name regex suggests that this field holds the regular expression itselfs, which will be confusing as a boolean can't hold such.

Bukama · 2020-04-01T18:40:21Z

src/main/java/org/junitpioneer/jupiter/DisableIfNameExtension.java

-		} else {
-			toDisable = context.getDisplayName().contains(details.value());
+
+		Optional<DisableIfDisplayName> disableIf = findAnnotation(testMethod, DisableIfDisplayName.class);


Please update to the new methods, provided by #187 after it is merged

Will wait for #187 to be merged to resolve all the other comments then

We just merged it, @nishantvas.

nipafx · 2020-04-07T20:19:27Z

Since @Bukama already a close look at this, I'm gonna abstain until his comments were addressed. Let me know when that's done, so I can give a final review.

Bukama · 2020-04-26T10:57:57Z

Hey @nishantvas,
do you know if you may find a moment to address my comments / requested changes?

nishant-vashisth · 2020-04-26T21:02:50Z

Hi @Bukama, apologies I've been occupied in some other things. And didn't have access to my PC.
I'll have this for review by tomorrow.
I appreciate your patience.

Bukama · 2020-04-27T05:32:13Z

Everything fine, take your time!

… into parameterized

Bukama

Thanks for the update. Just some small typos to correct and I would call this fine 👍

src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

Bukama · 2020-04-27T12:32:05Z

src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

+
+/**
+ * {@code @DisableIfDisplayName} is a JUnit Jupiter extension which can be used to
+ * selectively disable {@link ParameterizedTest} based on their {@link ExtensionContext#getDisplayName()}


Suggested change

* selectively disable {@link ParameterizedTest} based on their {@link ExtensionContext#getDisplayName()}

* selectively disable {@link ParameterizedTest} based on their {@link ExtensionContext#getDisplayName()}.

Bukama · 2020-04-27T12:32:43Z

src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

+ * selectively disable {@link ParameterizedTest} based on their {@link ExtensionContext#getDisplayName()}
+ *
+ * <p>
+ * The extension is an {@link ExecutionCondition} which validates dynamically registered tests


Suggested change

* The extension is an {@link ExecutionCondition} which validates dynamically registered tests

* The extension is an {@link ExecutionCondition}, which validates dynamically registered tests.

Bukama · 2020-04-27T12:33:18Z

src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

+ * This is highly useful since current {@link Disabled} or {@link DisabledIf} annotations disable
+ * the whole test but not the Parameterized tests selectively


Suggested change

* This is highly useful since current {@link Disabled} or {@link DisabledIf} annotations disable

* the whole test but not the Parameterized tests selectively

* This is highly useful since current {@link Disabled} or {@link DisabledIf} annotations disable

* the whole test, but not the Parameterized tests selectively.

Bukama · 2020-04-27T12:34:12Z

src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

+ * the whole test but not the Parameterized tests selectively
+ *
+ * If it is required to disable selective tests out of the plethora of dynamically
+ * registered Parameterized tests, then we can utilize the following


Suggested change

* registered Parameterized tests, then we can utilize the following

* registered Parameterized tests, then we can utilize the following.

Bukama · 2020-04-27T12:35:24Z

src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

+ * Each repeatable annotation will be processed for each test and test will be skipped if
+ * any of them evaluates true against the display name


Suggested change

* Each repeatable annotation will be processed for each test and test will be skipped if

* any of them evaluates true against the display name

* Each repeatable annotation will be processed for each test and the test will be skipped if

* any of them evaluates true against the display name.

Bukama · 2020-04-27T12:36:57Z

src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

+	 * Display names of the test cases to be disabled. The whole test case name can be passed as well as sub string
+	 * The values will be evaluated with {@link String#contains(CharSequence)} by default


Suggested change

* Display names of the test cases to be disabled. The whole test case name can be passed as well as sub string

* The values will be evaluated with {@link String#contains(CharSequence)} by default

* Display names of the test cases to be disabled. The whole test case name can be passed as well as a sub string.

* The values will be evaluated with {@link String#contains(CharSequence)} by default.

Co-Authored-By: Matthias Bünger <Bukama@users.noreply.github.com>

…ioneer into parameterized � Conflicts: � src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

Bukama

Thanks!

nishant-vashisth · 2020-04-27T16:07:06Z

@Bukama, thanks for the proof checking.

sonarcloud · 2020-05-19T21:01:20Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
1 Code Smell

100.0% Coverage
0.0% Duplication

nipafx · 2020-05-19T21:07:53Z

Thanks @nishantvas for the extension, this is really nifty. I added a bunch of changes to the PR, most notably that we can now distinguish between deactivating the test container and the individual invocations. While the code looked like it checked that (with getTestMethod) that didn't work for me. If you're ok with my changes, there are two more things that need to be discussed/done:

This belongs into the params package, but that's a problem because it uses our internal utilities. We (i.e. @junit-pioneer/junit-pioneer-team) have to come up with a solution for this.
Instead of value and isRegExp I'd prefer attributes contains and matches, e.g. @DisableIfDisplayName(contains = { "1", "2" }) or @DisableIfDisplayName(matches = ".*disabled?\\s.*"). What do you think?

nishant-vashisth · 2020-05-21T16:17:03Z

@nicolaiparlog

if (!context.getUniqueId().contains("test-template-invocation"))
			return enabled("Never disable parameterized test method itself");

This does look relatively neater than using the getTestMethod(), didn't know about it. Thanks.

Also, the matches / contains do make sense. But I didn't want to do that instead of value because in this case, we'll have to default both or one of these to { } , which can lead to people adding the annotation on a test with it doing nothing or us having to make one of these fields mandatory. The neater solution to do this would be to have two annotations which felt like an overkill at the time

Also, not using the keyword value does make it a bit more readable but the annotation name is descriptive to it's means
@DisableIfDisplayName("something") However, of course
@DisableIfDisplayName(contains = { "1", "2" }) looks equally if not perhaps more descriptive.

sonarcloud · 2020-06-20T14:54:03Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
4 Code Smells

92.3% Coverage
0.0% Duplication

nipafx · 2020-06-20T14:55:56Z

Regarding my two points:

I moved the classes and found a working, yet ugly solution for the utils problem. Not great, but better than exposing internal APIs to our users. Unless somebody has another idea how to keep utils package-visible without duplicating a lot of code (and tests?), I'll stick with this.
I implemented contains and matches attributes and think that it really helped with usability, although it does make the implementation a bit more cumbersome. We do get a nice feature on top, though: We can not combine substring search and regexp matching. Let me know what you think.

If nobody requires any further changes, I'm ok with this as it is and we can merge it. (Finally, sorry for taking so long with this.) Proposed commit message:

Disable parameterized test invocations by display name (#163 / #175)

Jupiter's `@Disabled`-based annotations disable entire test template
methods and can thus not be used to disable individual parameterized
test invocations. This extension adds that capability by matching
based on display name. Display names can be evaluated by searching
for substrings or matching against regular expressions (or both).

Because this extension belongs into the `params` package but also
needs to access `PioneerAnnotationUtils`, we needed to find a way
to make them accessible without making the class public. To prevent
duplication, we went with a (clever? abominable?) reflection hack.

Closes: #163
PR: #175

Bukama · 2020-06-20T15:09:37Z

As said in stream chat I agree with Pioneer. So I'm fine with the current state of the PR too.

Bukama

(Renewing acceptance)

nishant-vashisth · 2020-06-20T15:20:19Z

Yea the code change seems perfectly alright. The run time check for no values with annotation usage is quite alright. As long as it's documented. Otherwise it can be a tad misleading.

But I'm happy with this. Thanks for the changes

Add extension to disable per test name to be able to disable paramete…

55d43c1

…rized tests

nishant-vashisth mentioned this pull request Feb 19, 2020

Disable Parameterised tests selectively #163

Closed

Misplaced file

f23975a

nipafx mentioned this pull request Mar 3, 2020

Disable parameterised tests selectively #170

Closed

Add a test that is not disabled

c0aa200

nipafx reviewed Mar 26, 2020

View reviewed changes

src/main/java/org/junitpioneer/jupiter/DisableIfNameExtension.java Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/junit-pioneer/junit-pioneer…

d311db8

… into parameterized

Add documentation and Expand to test name as array

b84b49b

nishant-vashisth requested a review from nipafx March 30, 2020 18:04

nishant.vashisth added 2 commits March 30, 2020 20:10

Check Style

1852e35

Merge branch 'master' of https://github.com/junit-pioneer/junit-pioneer…

ca21927

… into parameterized

Bukama added the 🏗️ type: enhancement label Apr 1, 2020

Bukama requested changes Apr 1, 2020

View reviewed changes

nishant.vashisth added 2 commits April 27, 2020 13:13

Merge branch 'master' of https://github.com/junit-pioneer/junit-pioneer…

2e97fd0

… into parameterized

Update to new Annotation utils and grammar check docks

1b06a9d

Move tests to new Runner

b01381e

nishant-vashisth requested a review from Bukama April 27, 2020 11:39

Bukama requested changes Apr 27, 2020

View reviewed changes

nishant-vashisth and others added 3 commits April 27, 2020 16:49

Update src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

73bcfcf

Co-Authored-By: Matthias Bünger <Bukama@users.noreply.github.com>

Update typos

02bde4a

Merge branch 'parameterized' of https://github.com/nishantvas/junit-p…

f851041

…ioneer into parameterized � Conflicts: � src/main/java/org/junitpioneer/jupiter/DisableIfDisplayName.java

nishant-vashisth requested a review from Bukama April 27, 2020 15:28

Bukama approved these changes Apr 27, 2020

View reviewed changes

nipafx removed the 🏗️ type: enhancement label May 2, 2020

Nicolai Parlog added 4 commits May 19, 2020 22:54

Improve documentation

a649819

Refactor and add tests

a047ee1

Refactor extension

aca01b6

Don't accidentally disable entire test method

55e1de5

Nicolai Parlog added 6 commits June 20, 2020 15:43

Move new classes into params package

89e35e9

Add attributes contains and matches.

a591915

Apply Spotless

3c8486a

Update documentation

659942c

Fix failing test and move test code around

a602965

Apply Spotless [AGAIN!]

cb605ba

Bukama self-requested a review June 20, 2020 15:09

Bukama approved these changes Jun 20, 2020

View reviewed changes

Bukama merged commit 6c4056d into junit-pioneer:master Jun 20, 2020

		@@ -0,0 +1,42 @@
		:page-title: @DisableIfDisplayName
		:page-description: JUnit Jupiter extensions to selectively disable Parameterized tests

	* selectively disable {@link ParameterizedTest} based on their {@link ExtensionContext#getDisplayName()}
	* selectively disable {@link ParameterizedTest} based on their {@link ExtensionContext#getDisplayName()}.

	* The extension is an {@link ExecutionCondition} which validates dynamically registered tests
	* The extension is an {@link ExecutionCondition}, which validates dynamically registered tests.

		* This is highly useful since current {@link Disabled} or {@link DisabledIf} annotations disable
		* the whole test but not the Parameterized tests selectively

	* registered Parameterized tests, then we can utilize the following
	* registered Parameterized tests, then we can utilize the following.

		* Each repeatable annotation will be processed for each test and test will be skipped if
		* any of them evaluates true against the display name

		* Display names of the test cases to be disabled. The whole test case name can be passed as well as sub string
		* The values will be evaluated with {@link String#contains(CharSequence)} by default

Add extension to disable per test name to be able to disable parameterized tests #175

Add extension to disable per test name to be able to disable parameterized tests #175

Conversation

nishant-vashisth commented Feb 19, 2020 • edited

nipafx commented Mar 3, 2020

nishant-vashisth commented Mar 3, 2020

nipafx commented Mar 26, 2020

nipafx left a comment

Choose a reason for hiding this comment

nipafx commented Mar 26, 2020

nipafx commented Mar 26, 2020

Bukama commented Mar 26, 2020

Bukama commented Mar 26, 2020

nishant-vashisth commented Mar 26, 2020 • edited by nipafx

Bukama commented Mar 27, 2020

nipafx commented Mar 29, 2020

nishant-vashisth commented Mar 30, 2020

Bukama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nipafx commented Apr 7, 2020

Bukama commented Apr 26, 2020

nishant-vashisth commented Apr 26, 2020

Bukama commented Apr 27, 2020

Bukama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bukama left a comment

Choose a reason for hiding this comment

nishant-vashisth commented Apr 27, 2020

sonarcloud bot commented May 19, 2020

nipafx commented May 19, 2020

nishant-vashisth commented May 21, 2020

sonarcloud bot commented Jun 20, 2020

nipafx commented Jun 20, 2020 • edited

Bukama commented Jun 20, 2020

Bukama left a comment

Choose a reason for hiding this comment

nishant-vashisth commented Jun 20, 2020

nishant-vashisth commented Feb 19, 2020 •

edited

nishant-vashisth commented Mar 26, 2020 •

edited by nipafx

nipafx commented Jun 20, 2020 •

edited