junit-team / junit5

✅ The 5th major version of the programmer-friendly testing framework for Java and the JVM
https://junit.org
Other
6.32k stars 1.47k forks source link

Support `DiscoverySelectors.selectMethod(String)` without declaring parameters #3940

Open stdedos opened 2 weeks ago

stdedos commented 2 weeks ago

Hello there! Can I use https://junit.org/junit5/docs/current/api/org.junit.platform.engine/org/junit/platform/engine/discovery/DiscoverySelectors.html#selectMethod(java.lang.String) and "skip" declaring parameters that my fqdn#method may or may not take?

It is a manual orchestration, that makes declaring the parameters very tedious.

I don't mind it if it happens to match more than one method.

sbrannen commented 2 weeks ago

Hi @stdedos,

It seems you ignored the following when creating this issue.

This issue tracker is not the place for questions. If you want to ask how to do something, or to understand why something isn't working the way you expect it to, please use Gitter [1] or Stack Overflow [2].

[1] https://gitter.im/junit-team/junit5 [2] https://stackoverflow.com/questions/tagged/junit5

Thus, have you tried this yourself?

The reason I'm asking is that we would be grateful if people tried things before asking if something works. Otherwise, such issues waste people's time.

In any case, if I recall correctly, selectMethod() is used to select a specific method. In other words, there is no fuzzy logic regarding which method it selects.

If you leave off the parameter types, it's the same as specifying () which signals that the desired method does not declare any parameters.

It is a manual orchestration, that makes declaring the parameters very tedious.

I don't mind it if it happens to match more than one method.

Well, if it selected more than one method, that would be wrong, since its contract is to select a single method.

Having said that, we do actually have some fuzzy logic for selecting a method by fully-qualified name with @MethodSource in parameterized tests. For @MethodSource if only the method name is supplied, we find a single method with a matching name, ignoring the parameters, and we throw an exception if more than one method with that name exists.

So, I suppose we could consider introducing support similar to that for DiscoverySelectors.selectMethod().

Would that suit your needs?

stdedos commented 2 weeks ago

It seems you ignored the following when creating this issue.

No, not really 😅

https://matrix.to/#/!fHBBchhfNCUIhAwZZZ:gitter.im/$rmJqMnfnQI-EEo3OwJ8CU3lTCvCH2KPCENzObKsAEtc?via=gitter.im&via=matrix.org&via=gnome.org

(PS: "edit" my OP)

Thus, have you tried this yourself?

Yes, I have. I have called selectMethod("fqdn#method") via Ruby/JRuby, and it errors with "no such fqdn method found".

If you leave off the parameter types, it's the same as specifying () which signals that the desired method does not declare any parameters.

From the exception it seems like it, yes 😅

So, I suppose we could consider introducing support similar to that for DiscoverySelectors.selectMethod().

Would that suit your needs?

I don't mind even if it was selectFuzzyMethod(, selectMethodIgnoresParams(, selectMethods(, ... 😅

I don't see a more "generic" way to match parameters (from my limited understanding, hence the question), and I cannot have automation helping me on this one (like e.g. an IDE action that introspects the code and outputs the appropriate format).

sbrannen commented 1 week ago

It seems you ignored the following when creating this issue.

No, not really 😅

matrix.to/#/!fHBBchhfNCUIhAwZZZ:gitter.im/$rmJqMnfnQI-EEo3OwJ8CU3lTCvCH2KPCENzObKsAEtc?via=gitter.im&via=matrix.org&via=gnome.org

OK. Thanks for letting us know.

For the record, your original title and wording of the description made it sound like you were asking a simple "how does this work?" question rather than making a proposal.

In the future, it would be helpful if you pointed out that you had already tried it and were proposing an enhancement.

I don't mind even if it was selectFuzzyMethod(, selectMethodIgnoresParams(, selectMethods(, ... 😅

I don't see a more "generic" way to match parameters (from my limited understanding, hence the question), and I cannot have automation helping me on this one (like e.g. an IDE action that introspects the code and outputs the appropriate format).

I imagine we could introduce something like one of the following (likely also with String variants for supplying the fully-qualified class name instead of the Class).

The Predicate<Method> variant would of course be more flexible but impossible to support via the command line (i.e., as text); whereas, the String methodNamePattern variant would be limited to a simple globbing pattern or RegEx but would be easy to support as text.

Thoughts?

stdedos commented 1 week ago

For the record, your original title and wording of the description made it sound like you were asking a simple "how does this work?" question rather than making a proposal.

In the future, it would be helpful if you pointed out that you had already tried it and were proposing an enhancement.

I have never used JUnit "from the developer's viewpoint" (only for running/writing tests). While I did some initial investigation, my perspective might have been incomplete 😓

I "guessed" that you should have some more versatile methods that do not require specifying everything. I understand there might be valid reasons why callers must define everything (e.g., maybe it's trivial for an IDE using selectMethod).

So yes, this began as a "probing question" in Matrix, moved here, and evolved into an accepted enhancement proposal 🎉

The Predicate<Method> variant would of course be more flexible but impossible to support via the command line (i.e., as text); whereas, the String methodNamePattern variant would be limited to a simple globbing pattern or RegEx but would be easy to support as text.

In my case, I want to use user input to it - I cannot think how to use the Predicate<Method> predicate effectively.

String methodNamePattern sounds nice. I get from this that e.g. from f.q.d.n#method(f.q.d.n.User), any of

would select it? That looks amazing 😍 (NB: Contrived examples for the sake of bouncing ideas)

I would consider whether auto-surrounding with * a glob pattern that contains no globing modifiers would be suitable (i.e., giving testUniqueShouldReturnValidResponseWhenAllInputsAreProvided as input would select the same-named method)

Will anything else be supported? Note that I am not trying to blow up the scope - the smallest, fastest, "more aligned to the project's goals".

Please consider writing (all) the usage examples you can think of in the documentation (and/or test them) 🙏

sbrannen commented 1 week ago

I "guessed" that you should have some more versatile methods that do not require specifying everything. I understand there might be valid reasons why callers must define everything (e.g., maybe it's trivial for an IDE using selectMethod).

DiscoverySelectors provides various options for selecting tests, with varying granularity.

Normally, people need to be able to select all test methods within a test class via one of the selectClass(...) selectors, or they need to be able to select individual methods via one of the selectMethod(...) selectors.

I don't recall anyone ever having asked for the ability to select multiple methods based on a single "input" (for example, a pattern). Though, I can see how that could be useful.

So yes, this began as a "probing question" in Matrix, moved here, and evolved into an accepted enhancement proposal 🎉

Well, we have not yet officially "accepted" it (hence no milestone assignment), but we are certainly open to exploring the possibilities. 😉

In my case, I want to use user input to it - I cannot think how to use the Predicate<Method> predicate effectively.

If we go with any sort of official "pattern" support, that would almost certainly delegate to the Predicate support. In other words, applying a pattern to a Method is a specialization of Predicate<Method>. A predicate could be as simple as method -> method.getName().equals("testFoo") (for an exact match) or method -> method.getName().matches(".+Foo") (for a RegEx match).

String methodNamePattern sounds nice. I get from this that e.g. from f.q.d.n#method(f.q.d.n.User), any of

  • f.q.d.n#method(f.q.d.n.User)
  • /^f.q.d.n#me/
  • f.q.d.n#met*
  • method(*User)
  • *#method(*User (Which looks like a way to me to get the "best" of both worlds: I can "uniquely" select a test, by specifying only its name and/or its first parameter - while skipping all fqdns)

would select it? That looks amazing 😍 (NB: Contrived examples for the sake of bouncing ideas)

Thanks for brainstorming!

Those are certainly some interesting ideas.

while skipping all fqdns

Regarding that, I don't foresee how that would be possible. We'd have to have a Class or fully-qualified class name to use as a starting point when searching for methods. Otherwise, we (or test engines) would have to scan every single class in the classpath, which is not an option.

Will anything else be supported?

That remains to be seen.

I'll add a follow-up comment to address some of my concerns about implementation details.

Note that I am not trying to blow up the scope - the smallest, fastest, "more aligned to the project's goals".

Understood.

Please consider writing (all) the usage examples you can think of in the documentation (and/or test them) 🙏

Yes, we would definitely document example usage.

sbrannen commented 1 week ago

I imagine we could introduce something like one of the following (likely also with String variants for supplying the fully-qualified class name instead of the Class).

  • List<MethodSelector> selectMethods(Class<?> javaClass, Predicate<Method> predicate)
  • List<MethodSelector> selectMethodsByPattern(Class<?> javaClass, String methodNamePattern)

After putting a bit more thought into it, I have some concerns about the above proposal.

A MethodSelector selects a single method, and it does that lazily (in the case of a Java Method) in order to support method selection across varying programming languages.

It's up to a TestEngine to decide how to process a MethodSelector. For example, the Jupiter test engine relies on MethodSelector.getJavaMethod() to resolve the selector to a single java.lang.reflect.Method.

So, by the time a test engine processes a MethodSelector, the selector should already know how to resolve a single method, and it cannot do that lazily based on a Predicate or pattern since that may end up selecting multiple matching methods.

To support predicates/patterns while simultaneously ensuring that a MethodSelector selects a single method, the new selectMethods/selectMethodsByPattern methods in DiscoverySelectors would have to eagerly resolve the methods.

That might work, but it would mean that this feature would only work for Java methods. In other words, it would not be a generic "select methods by pattern" feature that could be used for other programming languages or other use cases.

Thus, in order to provide generic support for selecting potentially multiple methods based on a pattern or predicate, I think we might need to introduce a new type of DiscoverySelector – for example, a MethodsSelector (note the "s") with a different API than the MethodSelector API.

But... a new type of DiscoverySelector always requires explicit support in test engines (for example, JUnit Jupiter, JUnit Vintage, Spock, etc.) as well as in the org.junit.platform.engine.support.discovery.SelectorResolver.

In other words, if we introduce a new DiscoverySelector or modify the existing MethodSelector, that doesn't mean the new feature would actually be supported by any existing test engines.

stdedos commented 1 week ago

Regarding that, I don't foresee how that would be possible. We'd have to have a Class or fully-qualified class name to use as a starting point when searching for methods. Otherwise, we (or test engines) would have to scan every single class in the classpath, which is not an option.

I see. At least I would hope that every half-decent IDE can provide a "Copy Reference" action (though it seems to not focus on arguments) - so fqdn would be fine.

I wonder if a minified but unique FQDN would work for "less manual typing": e.g. c.ex.u in place of com.example.utils (again without overshooting scope)

After putting a bit more thought into it, I have some concerns about the above proposal.

...

If it looks like a full development work rather than "patchwork", then of course please weight cost/benefits 🙏

It seems we are using org.junit.platform.launcher.core.LauncherFactory.create to .execute(request) which is prepared via DiscoverySelectors / LauncherDiscoveryRequestBuilder (if that somehow helps further)

sbrannen commented 1 week ago

I see. At least I would hope that every half-decent IDE can provide a "Copy Reference" action (though it seems to not focus on arguments) - so fqdn would be fine.

Yes, I think most modern IDEs provide that functionality.

I wonder if a minified but unique FQDN would work for "less manual typing": e.g. c.ex.u in place of com.example.utils (again without overshooting scope)

That would require scanning of the classpath to find candidate classes, which I still think would be a no-go (at least as a first-class feature in JUnit).

If it looks like a full development work rather than "patchwork", then of course please weight cost/benefits 🙏

To provide generic support suitable for use with different test engines and programming languages, it might turn out to be "full development work"; however, if you wanted to implement it yourself, it turns out it's not that hard. I'll demonstrate in a subsequent comment.

sbrannen commented 1 week ago

Luckily the factory methods in DiscoverySelectors can be used as building blocks for custom needs.

Along those lines, I spiked a custom implementation of selectMethods(String) that supports regular expression matching for the method name (ignoring parameters for the sake of simplicity).

Given the following test class...

package example;

class ExampleTestCase {
    @Test
    void aardvark() {
    }

    @Test
    void anaconda() {
    }

    @Test
    void bat() {
    }

    @Test
    void cat() {
    }
}

... and the following ...

import java.lang.reflect.Method;
import java.util.function.Predicate;

import org.junit.jupiter.api.Test;
import org.junit.platform.commons.PreconditionViolationException;
import org.junit.platform.commons.support.HierarchyTraversalMode;
import org.junit.platform.commons.support.ReflectionSupport;
import org.junit.platform.commons.util.ReflectionUtils;
import org.junit.platform.engine.TestDescriptor;
import org.junit.platform.engine.discovery.DiscoverySelectors;
import org.junit.platform.engine.discovery.MethodSelector;
import org.junit.platform.testkit.engine.EngineTestKit;
import org.junit.platform.testkit.engine.Event;

class SelectMethodsByPatternTests {

    @Test
    void verifyAllJupiterEvents() {
        EngineTestKit.engine("junit-jupiter")//
                .selectors(selectMethods("example.ExampleTestCase#(aa.+|bat)"))//
                .execute()//
                .testEvents()//
                .finished()//
                .stream()//
                .map(Event::getTestDescriptor)//
                .map(TestDescriptor::getDisplayName)//
                .sorted()
                .forEach(System.err::println);
    }

    public static MethodSelector[] selectMethods(String fullyQualifiedMethodNamePattern) {
        String[] methodParts = ReflectionUtils.parseFullyQualifiedMethodName(fullyQualifiedMethodNamePattern);
        String className = methodParts[0];
        Class<?> javaClass = ReflectionSupport.tryToLoadClass(className)//
                .getOrThrow(
                    cause -> new PreconditionViolationException("Could not load class with name: " + className, cause));
        String methodNamePattern = methodParts[1];
        Predicate<Method> predicate = method -> method.getName().matches(methodNamePattern);

        return ReflectionSupport.streamMethods(javaClass, predicate, HierarchyTraversalMode.TOP_DOWN)//
            .map(method -> DiscoverySelectors.selectMethod(javaClass, method))
            .toArray(MethodSelector[]::new);
    }

}

Running that runs/prints the following to the console, thanks to the custom selector: selectMethods("example.ExampleTestCase#(aa.+|bat)").

aardvark()
bat()

Using selectMethods("example.ExampleTestCase#.+") runs/prints:

aardvark()
anaconda()
bat()
cat()

etc.

stdedos commented 1 week ago

Given how simple this looks, I wonder if I should spike a layer, that would "do what I want it to do", and use that as an input to the current API instead 🤔

sbrannen commented 1 week ago

Given how simple this looks, I wonder if I should spike a layer, that would "do what I want it to do", and use that as an input to the current API instead 🤔

YES!

That's certainly a viable option and perhaps what the team may end up recommending in the end. 😉

In any case, this is flagged for "team discussion", and we'll discuss it in the coming weeks.

But... beware: ReflectionUtils.parseFullyQualifiedMethodName() is not supported. So you'd be better off implementing your own "split" functionality for that.

stdedos commented 1 week ago

But... beware: ReflectionUtils.parseFullyQualifiedMethodName() is not supported. So you'd be better off implementing your own "split" functionality for that.

WDYM? 😅

Worst-case, I can just ... copy-paste the code and use it, right? 😕 (or idk what the issue is, and I am "answering" a different question in my head)

sbrannen commented 1 week ago

WDYM? 😅

ReflectionUtils is officially not supported.

That means that we reserve the right to change and/or remove any functionality whenever we deem it appropriate.

ReflectionSupport, on the other hand, is officially supported. But... we don't have any such parseFullyQualifiedMethodName() there.

In other words, it's "use at your own risk", as the Javadoc states.

Worst-case, I can just ... copy-paste the code and use it, right?

It of course depends on how you you plan to use it, based on the license, but I am not a lawyer and cannot give advice about licenses, etc.

In any case, you can literally just split the input string as you see fit -- for example, using String#split, String#substring, etc.

stdedos commented 1 week ago

It of course depends on how you you plan to use it

I am planning to ... not distribute it. So I should be good, right? 😅

(I heard the IANAL 👌)

That means that we reserve the right to change and/or remove any functionality whenever we deem it appropriate. ... In any case, you can literally just split the input string as you see fit -- for example, using String#split, String#substring, etc.

Keeping these in mind 👍

stdedos commented 1 week ago

It seems that I can spike off your spike, and more or less get the result I want. I guess tl;dr I was looking for ReflectionSupport.streamMethods.

Thank you for your continuous support!

Now, if I could spike something "crazy" for ReflectionSupport.tryToLoadClass like "parse minified fqdn", and/or downright "just find where this method is declared" 😍 ... that'd be awesome.

(of course, I'd be additionally interesting to compare implementations performance-wise 😅)

stdedos commented 1 week ago

btw, two questions: