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

Disabled Extensions #1

Closed
8 tasks done
nipafx opened this issue Nov 19, 2016 · 31 comments
Closed
8 tasks done

Disabled Extensions #1

nipafx opened this issue Nov 19, 2016 · 31 comments
Labels
⚙️ component: Jupiter Issues coming from (internal/official) Jupiter features etc. 🏗️ type: enhancement 📢 up for grabs

Comments

@nipafx
Copy link
Member

nipafx commented Nov 19, 2016

There are many possible ideas floating around the @Disabled annotation:

It is conceivable to create a universal @DisabledIf annotation that covers all of these cases but instead of going there from the outset, it might make more sense to tackle individual use cases first and look for room for generalization later.

@sbrannen
Copy link

it might make more sense to tackle individual use cases first and look for room for generalization later.

👍

In case you're not aware of it, Spring 5 milestones already support @EnabledIf and @DisabledIf variants that accept SpEL expressions which make the implementation of custom conditions like @DisabledOnWindows extremely simple.

@nipafx
Copy link
Member Author

nipafx commented Nov 20, 2016

Thanks for the hint (links: EnabledIf, DisabledIf), didn't know about that. I'd like to keep dependencies to a minimum but I'll think about it.

Regarding generalizations, I was exploring ideas to get the compiler to check conditions. But I'll start with the simple cases...

@sbrannen
Copy link

Thanks for the hint (links: EnabledIf, DisabledIf), didn't know about that.

You're welcome.

I'd like to keep dependencies to a minimum but I'll think about it.

Sure. I wasn't implying that you should re-implement or copy them. Rather, I meant for them to serve as possible inspiration. Although supporting an expression language of any sort in core JUnit would naturally require the EL implementation to be an optional dependency. Anyway, just food for thought.

Regarding generalizations, I was exploring ideas to get the compiler to check conditions.

Can you expound on that?

@nipafx
Copy link
Member Author

nipafx commented Nov 22, 2016

Spawned #2 and added as a task to the issue description.

@nipafx nipafx mentioned this issue Nov 22, 2016
@nipafx
Copy link
Member Author

nipafx commented Nov 22, 2016

Spawned #3 to continue discussion about generalized conditions there.

@sormuras
Copy link
Member

A DisabledOn("java.version", "^9.*") would be convenient to have here: https://github.com/junit-team/junit5/pull/598/files -- with first parameter pointing to a System property key and the second parameter is a regex pattern to match.

@nipafx
Copy link
Member Author

nipafx commented Dec 23, 2016

Good point about the Java version, I added an item to the checkbox list in the issue description.

@jbduncan
Copy link
Contributor

jbduncan commented Mar 8, 2017

My knowledge of annotations is pretty limited, so I've no idea if what I'm going to suggest is even feasible, but here goes!

How about a @DisabledIf(BooleanSupplier) annotation that disables if the supplier evaluates to true, but doesn't if it evaluates to false?

@nipafx
Copy link
Member Author

nipafx commented Mar 8, 2017

@jbduncan: I considered something like that and was halfway done implementing it when I realized that annotations can only use constants as fields. Now I'm toying with the though to name a static field that us a BooleanSupplier and gets evaluated.

@nipafx
Copy link
Member Author

nipafx commented Jul 2, 2017

@smoyer64 [wrote]:

We should probably have a discussion about the disabling extensions as I have some that overlap. I started with @enableif and @DisableIf with a predicate as the value and another String as a message template. Then I built everything else on that. I have @EnableIfOs (and disable) in the middle and specific annotations at the end.

This code is not done but I like the generic nature of the extension and how you add new annotations with only their associated classes. I'll get what I have checked into the old selesy project so you can assess it.

That is excellent! As you can see above (particularly here) I started on the specific end but planned to go that very direction. I did some experiments and also tried to support predicates. I failed to find a way around the fact that you can not define a lambda as an annotation argument and found no other solution I immediately liked. I'm curious to see which way you went.

@nipafx
Copy link
Member Author

nipafx commented Jul 2, 2017

Just had this idea: An enabled/disabled extension with which you can form Boolean formulas over existing extensions: @DisabledIf(and(EnabledOnLinux.class, DisabledOnJava9.class)).

@smoyer64
Copy link
Member

smoyer64 commented Jul 2, 2017 via email

@smoyer64
Copy link
Member

smoyer64 commented Jul 6, 2017

@nicolaiparlog I've committed the POC code I had for @DisabledIf along with a predicate for OS and a meta-annotation (@DisabledIfOperatingSystemIsLinux). This code isn't anywhere near done and it certainly needs some tests written if we decide to go this way but it does allow the user to write new predicates that determine enabling or disabling. As I noted in the commit, @EnableIf is simply the logical negation of the @DisabledIfExtension.

See https://github.com/selesy/junit5-extensions/blob/develop/junit5-extensions-disable-rest-extension/src/main/java/com/selesy/testing/junit5/extensions/disable/rest/os/DisabledIfOperatingSystemIsLinux.java

@ttddyy
Copy link

ttddyy commented Jul 7, 2017

Hi @smoyer64

As @sbrannen mentioned, spring already uses @DisabledIf annotation.
From user's perspective, it would be nice that annotation names don't collide.
If same names are used, users need to check the import statement for which @DisabledIf is used or need to use full package name especially both @DisabledIf annotations are used in single file.

So, just IMO for junit-pioneer, naming @DisabledIf to @DisabledWhen or similar might be a tiny good thing for users.

@sbrannen
Copy link

sbrannen commented Jul 7, 2017

I'm afraid that multiple projects in this world will come up with an annotation named @DisabledIf.

TBH, I tried to come up with a better name for Spring, but in the end I figured @DisabledIf was better than something like @DisabledIfSpel.

So I think the conflicts are unavoidable.

In any case, it's definitely something to keep in mind.

@sbrannen
Copy link

sbrannen commented Jul 7, 2017

@nicolaiparlog, FYI: there's been a very simplistic PoC for a system property condition here for quite some time. That also serves as a PoC for an environment variable condition. Of course, I'd come up with a better name than @SystemProperty if I were to publish it.

@smoyer64
Copy link
Member

smoyer64 commented Jul 7, 2017

@ttddyy - I think the name is apropos of its function and don't mind that it collides. There's a large population of JUnit users that doesn't use Spring and some words are just right - even early versions of the JDK had java.util.Date and java.sql.Date.

I've also got a set of temporal conditions that are tentatively using @DisabledBefore, @DisabledAfter and @DisabledBetween.

Unfortunately, naming is hard - I do like the fact that the Spring version accepts EL and that was something I was considering after Java 9 (we should be able to exec code then). I think the real question is whether we can create something that's worth importing into a Spring project!

@sbrannen
Copy link

sbrannen commented Jul 7, 2017

I think the real question is whether we can create something that's worth importing into a Spring project!

Yep.... that will be the real challenge!

Because... with SpEL support in Spring's @EnabledIf, you can basically implement 90% of the conditions proposed in this issue as one-liners, like @EnabledOnMac, which basically just boils down to the following one-liner (if you don't want a custom annotation).

@EnabledIf(expression = "#{systemProperties['os.name'].toLowerCase().contains('mac')}", reason = "Enabled on Mac OS")

@mkobit
Copy link

mkobit commented Jul 29, 2017

I wonder if something similar to how @ParameterizedTest is structured would be useful here? Then different implementations of enable if and disable if could be implemented by consumers with not too much effort.

Incomplete code below, but would allow for the solution up above (I think) as well as other possible implementations (System property, environment variable, etc.):

public class DisabledIfConditionalExecution implements ExecutionCondition {

  @Override
  ConditionEvaluationResult evaluateExecutionCondition(ExtensionContext context) {
    // ...
  }
}

public @interface DisabledIf {
}

public @interface DisabledIfSource {
  Class<? extends DisabledIfProvider> value();
}

public interface DisabledIfProvider {
  boolean isDisabled(ExtensionContext context);
}

@DisabledIfSource(DisabledIfSpelProvider.class)
@interface DisabledIfSpel {
  String expression();
  String reason();
}

class DisabledIfSpelProvider implements DisabledIfProvider {
  boolean isDisabled(ExtensionContext context) {
    // ...
  }
}

class MyTest {
  @DisabledIfSpel(expression = "#{systemProperties['os.name'].toLowerCase().contains('mac')}", reason = "Enabled on Mac OS")
  void myTest() {
  }
}

@mkobit
Copy link

mkobit commented Sep 12, 2017

Related to junit-team/junit5#219 as well.

@nipafx nipafx added the ⚙️ component: Jupiter Issues coming from (internal/official) Jupiter features etc. label Jan 21, 2018
@sormuras
Copy link
Member

For the record and some-what related to this initial pioneer issue: there's a new label for "pioneer" ideas at https://github.com/junit-team/junit5/issues?q=is%3Aissue+is%3Aopen+label%3A%223rd-party%3A+pioneer%22 -- tagging issues that may be interesting for @junit-pioneer

@nipafx
Copy link
Member Author

nipafx commented Apr 25, 2018

@sormuras Can you take a look at my initial bullet point list and tell us which ideas Jupiter already stole implemented. 😉

@sbrannen
Copy link

I'm not @sormuras, but I'll update this issue's description to check off what's already covered. 😉

@sbrannen
Copy link

done

@nipafx
Copy link
Member Author

nipafx commented May 2, 2018

@sbrannen: Thank you for the input. 😺

@ALL: From the three unchecked ones, I consider each fair game, particularly @DisabledUntil. If anyone wants to tackle these, feel free to do so.

nipafx pushed a commit that referenced this issue May 2, 2018
@DisabledOnOs was superseeded my more extensive @disabled... support
in Jupiter. It can hence be removed from Pioneer.
nipafx pushed a commit that referenced this issue May 2, 2018
@DisabledOnOs was superseeded my more extensive @disabled... support
in Jupiter. It can hence be removed from Pioneer.
@nipafx nipafx removed this from the Collect Basic Features milestone May 2, 2018
@smoyer64
Copy link
Member

smoyer64 commented May 2, 2018

@nicolaiparlog The conditional disabling of tests was something we were both working in before joining forces - there's a @DisableRest in the portion of selesy/junit-extension I had asked you to review a while ago (https://github.com/selesy/junit5-extensions/tree/develop/junit5-extensions-disable-rest-extension/src/main/java/com/selesy/testing/junit5/extensions/disable/rest). Will this satisfy the "disable all but one" criteria? It's a bit more than that because if you annotate (e.g.) two of ten tests they'll both run.

I should also note that I don't really like the name - Rest is not REST but Roy Fielding has effectively defined the meaning of that word for developers. Perhaps @DisableOthers?

@marcphilipp
Copy link
Member

In the Javascript frameworks I‘ve used that‘s usually called only.

@nipafx
Copy link
Member Author

nipafx commented May 8, 2018

Opened #71 for @DisableOthers/@DisableRest/@EnableOnly/whatever.

@nipafx
Copy link
Member Author

nipafx commented Jun 20, 2020

At least the last two items from the opening post still sound interesting to me. If anybody else shares that, PRs are welcome. :)

@nipafx
Copy link
Member Author

nipafx commented Oct 27, 2020

@mkobit I never replied to your proposal above. I liked the idea a lot and was about to create an issue for it when I was pointed at Jupiter's custom conditions, which provide all the features your proposal does. Main difference: Jupiter's solution is a bit simpler, but yours is type safe and survives refactorings. I don't think that's enough to warrant our own implementation, so I won't create that issue.

@nipafx nipafx removed this from the Exploring Io milestone Oct 27, 2020
@nipafx
Copy link
Member Author

nipafx commented Oct 27, 2020

As I see it, we've covered all ideas in this issue, so we can finally close it. 🥳

@nipafx nipafx closed this as completed Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚙️ component: Jupiter Issues coming from (internal/official) Jupiter features etc. 🏗️ type: enhancement 📢 up for grabs
Projects
None yet
Development

No branches or pull requests

9 participants