junit-team / junit5

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

Introduce first-class support for scenario tests #48

Open sbrannen opened 8 years ago

sbrannen commented 8 years ago

Proposal

The current proposal is to introduce the following:

The @Step annotation will need to provide an attribute that can be used to declare the next step within the scenario test. Steps will then be ordered according to the resulting dependency graph and executed in exactly that order.

For example, a scenario test could look similar to the following:

@ScenarioTest
class WebSecurityScenarioTest {

    @Step(next = "login")
    void visitPageRequiringAuthorizationWhileNotLoggedIn() {
        // attempt to visit page which requires that a user is logged in
        // assert user is redirected to login page
    }

    @Step(next = "visitSecondPageRequiringAuthorizationWhileLoggedIn")
    void login() {
        // submit login form with valid credentials
        // assert user is redirected back to previous page requiring authorization
    }

    @Step(next = "logout")
    void visitSecondPageRequiringAuthorizationWhileLoggedIn() {
        // visit another page which requires that a user is logged in
        // assert user can access page
    }

    @Step(next = END)
    void logout() {
        // visit logout URL
        // assert user has been logged out
    }

}

Related Issues

mmerdes commented 8 years ago

@sbrannen: do you prefer referencing the next step by name? i assume these references would be refactoring-safe, so no real problem here. but what about typos? (i guess just using numbered steps will be awkward if you introduce steps in the middle...)

jlink commented 8 years ago

Is it in scope for Alpha1 (end of next week)?

bechte commented 8 years ago

Thanks for writing down the concept and providing a detailed example. I think it is great and I also like the next attribute pointing to the methodName. Two reasons:

I still wonder if we want to point to the next or rather to the previous step? From a reader's perspective, I would like to have an idea what are the preconditions for this step? Especially as the annotation stands above the method declaration, I would suggest using a "previous" or "ancester" attribute. But it's just a feeling.

Concerning the Alpha-M1: I don't think it will be part of it. We should probably move it to a different milestone. What do you think, @sbrannen ?

jlink commented 8 years ago

Why not use preconditions as the concept to determine the order? A step can have none, one or many. A runner could thereby parallelize test execution as much as possible. And one could determine the minimum set of steps that must be executed for a certain step to make sense at all.

2015-12-04 11:24 GMT+01:00 Stefan Bechtold notifications@github.com:

Thanks for writing down the concept and providing a detailed example. I think it is great and I also like the next attribute pointing to the methodName. Two reasons:

  • IDE refactoring of the method name should also replace the next attribute string
  • IDE vendors will have a nice and easy way to support auto-completion for steps

I still wonder if we want to point to the next or rather to the previous step? From a reader's perspective, I would like to have an idea what are the preconditions for this step? Especially as the annotation stands above the method declaration, I would suggest using a "previous" or "ancester" attribute. But it's just a feeling.

— Reply to this email directly or view it on GitHub https://github.com/junit-team/junit-lambda/issues/48#issuecomment-161930920 .

bechte commented 8 years ago

Yes, this will be a consequence of turning from "next" to "ancestor". Preconditions are great, but we need to be aware of the added complexity. Maybe we discuss this topic in person? Anyways, I like this feature. :+1:

dinesh707 commented 8 years ago

Referring a method by string sounds like way to do more mistakes and as @bechte commented it will not give IDE support when coding. I would suggest something like following

final String LOGIN_ID = "login_id"; final String LOGOUT_ID = "logout_id"; final String VISIT_PAGE_ID = "visit_page_id";

@Step(id = LOGOUT_ID, waitFor = VISIT_PAGE)

so the waitFor will wait till VISIT_PAGE is completed. That way executor should be able to plan concurrent executions as well.

But again I really see the benefit of the original proposal

sormuras commented 8 years ago

Utilizing a simple state machine (using enums instead of strings) is an overkill, right? Like https://github.com/oxo42/stateless4j or others.

bechte commented 8 years ago

I was trying to say that I like the method name references. I would like to avoid introducing another ID for the tests. I think IDE vendors are great for picking up method names (outline view, etc.) and they will easily support refactorings and support test writers with auto-completion. This can all happen on static code analysis and, therefore, is something they can support right-away.

At least as a test writer, I dont want to maintain the list of IDs. It's boilerplate anyways. We might think about support the test name:

@Test(name = "Step1") 

and allow referencing the name as well. But for a first alpha, I would stick with the easy method name approach provided by @sbrannen .

sbrannen commented 8 years ago

@jlink and @bechte, scheduling this issue for Alpha M1 was intentionally optimistic.

Whether or not it's possible for Alpha M1 really depends on how far we get with the rewrite of the JUnit 5 engine -- for example, whether we have extension points for test instance lifecycle and ordering of tests.

If we don't get far enough in Alpha M1, we will naturally push the implementation of this issue to a subsequent milestone. However, we need to make sure that everyone has this on his radar; otherwise, it will likely be more challenging to integrate this feature as an afterthought.

dsaff commented 8 years ago

If this wasn't first-class supported, how hard would it be to add via a current extension point?

David Saff

sbrannen commented 8 years ago

@mmerdes, @bechte, @jlink, @dinesh707,

Thanks for the feedback on ordering and dependencies!

The reason I have thus far only proposed an @Step annotation with a next attribute is to keep it as simple as possible for the first round of implementation.

To address the issues you have raised...

Depending on how we decide to support ordering in general (i.e., for standard @Test methods), we may find a common mechanism for ordering steps in a scenario test as well. For example, if we introduce something like @Order for @Test methods, we could optionally reuse that for @Step methods.

The difference is that an annotation like @Order is typically based on numerical ordering. If you initially, intentionally define gaps between the numbers you declare (e.g., 10, 20, 30 instead of 1, 2, 3), it is then possible to easily insert another test or step in one of those gaps. In that sense numerical ordering is rather flexible though at the same time not as precise. For steps in a scenario test, however, I imagine people will wish to be very precise about the dependencies between steps.

As for whether a step declares its dependency by referencing the previous step vs. the next step, I think it will become more clear what the better solution is once we begin to experiment more with this feature.

With regard to supporting a precondition mechanism, that would certainly increase the flexibility of step execution within a scenario test, but I think we should first focus on a simpler approach before adding the level of complexity that would be required to support that correctly.

Along the same line of thinking, for the initial draft of this feature I would recommend that we not concern ourselves with parallel execution of steps within a scenario test.

sbrannen commented 8 years ago

@dsaff,

With the current set of extension points, it would be impossible to implement a feature like this as an extension.

However, that may soon change, depending on the outcome of the current effort to redesign the test discovery and test execution models.

In general, our goal is to introduce as many extension points that we deem feasible. Thus, if we create the necessary extension points in the engine to make it possible to implement scenario tests as an extension, we would naturally opt to implement it as an extension even if the feature is available out-of-the-box. That's analogous to how we implemented @Disabled and @TestName.

sbrannen commented 8 years ago

@sormuras,

Yes, I think that using or implementing a true state machine would be overkill for this feature.

sbrannen commented 8 years ago

@bechte,

I think you meant:

@Name("Step1")
@Step

instead of:

@Test(name = "Step1")

Right?

bechte commented 8 years ago

@sbrannen True :+1: - Thanks ;-)

luontola commented 8 years ago

Here is an idea for an alternative way of defining test order, using the normal rules of code execution order: http://specsy.org/documentation.html#non-isolated-execution-model

That way requires that tests can be nested and that tests are declared as method calls which take a lambda as parameter, so it can be hard to fit it together with the JUnit style of declaring tests as method declarations.

sbrannen commented 8 years ago

@orfjackal, thanks for sharing the link to Specsy's Non-Isolated Execution Model.

shareSideEffects() does in fact support the same underlying runtime model that we aim to achieve; however, you are also correct that it does not align with the standard Java class and method based approach used in JUnit 4 and JUnit 5.

Since the JDK does not guarantee that methods are returned in the order in which they are declared when using reflection, we have to introduce an explicit mechanism to control the order. So unless we introduce an alternative programming model based solely on lambdas (which has in fact been discussed), we unfortunately won't be able to support a programming model as simple as Specsy in this regard.

thomasdarimont commented 8 years ago

Interesting discussion - I just wanted to throw in two alternative ideas that came to my mind:

One alternative for referring to test methods via Strings or explicit lambdas could be method-references. It's a "bit" more involved but would look less verbose than lambdas and could also be checked by the compiler. Here is an example for this: https://gist.github.com/thomasdarimont/eb1bc1d24ccf3decbdc6 (Though I admit that this is more of a hack...)

Another idea is to create a proxied mock object of the current test class that would allow you to specify the order of test executions via the invocation order of the mock methods. This is similar to what Mocking frameworks like mockito support but here one would describe the actual "execution plan" for the test:

    @ScenarioSetup
    public void simpleScenario(WebSecurityScenarioTest mock){

        mock.visitPageRequiringAuthorizationWhileNotLoggedIn();
        mock.login();
        mock.visitSecondPageRequiringAuthorizationWhileLoggedIn();
        mock.logout();
    }
jillesvangurp commented 8 years ago

Why not rename @ScenarioTest to @Test? I don't see the need for introducing a completely new annotation. Currently junit doesn't have any class level @Test, like testng does have. I've always thought this was a strange omission and junit 5 looks like it could be a nice moment to fix that. The meaning of this is simple: every public method in this class is a test (unless annotated with BeforeXXX/AfterXXX). This would eliminate a great deal of verbosity since it allows you to leave out redundant @Test declarations on each method.

I would also change the @Step annotation to @Next("testName"). The testName should match to either the methodName or the @Name if configured. For the last step, the next would not be needed.

Any test methods with @Next would be guaranteed to be executed in the chained order. Any other tests are executed before or after in whatever default order configured for junit. Together with the class level @Test this gives you the least verbose way to do scenario testing. Combined with nested tests as proposed for junit5 this could actually provide something that closely resembles rspec.

Also having chained test methods finally gives us something like @BeforeEach and @AfterEach because each chain implictly has a first and last method without requiring the use of static methods (which I've always found a very unfortunate decision in junit). But maybe, we can make this more explicit by introducing @BeforeScenario and @AfterScenario.

jlink commented 8 years ago

Sounsd like a good use case for a test engine of your own?

I wouldn't like to overload the meaning of @Test within the JUnit 5 engine.

Am 31.12.2015 um 11:16 schrieb Jilles van Gurp notifications@github.com:

Why not rename @ScenarioTest to @Test? I don't see the need for introducing a completely new annotation. Currently junit doesn't have any class level @Test, like testng does have. I've always thought this was a strange omission and junit 5 looks like it could be a nice moment to fix that. The meaning of this is simple: every public method in this class is a test (unless annotated with Before/After). This would eliminate a great deal of verbosity since it allows you to leave out redundant @Test declarations on each method.

I would also change the @Step annotation to @Next("testName"). The testName should match to either the methodName or the @Name if configured. For the last step, the next would not be needed.

Any test methods with @Next would be guaranteed to be executed in the chained order. Any other tests are executed before or after in whatever default order configured for junit. Together with the class level @Test this gives you the least verbose way to do scenario testing. Combined with nested tests as proposed for junit5 this could actually provide something that closely resembles rspec.

Also having chained test methods finally gives us something like @BeforeEach and @AfterEach without requiring the use of static methods like testng provides (which I've always found a very unfortunate decision in junit). But maybe, we can make this more explicit by introducing @BeforeScenario and @AfterScenario.

— Reply to this email directly or view it on GitHub.

jillesvangurp commented 8 years ago

In my view it wouldn't overload @Test at all but merely give it exactly the same meaning except at the class level; just like testng has provided for some time. In junit3, you could actually extend TestCase to achieve the same goal.

The second part of what I propose is a somewhat less verbose alternative to what @sbrannen proposed and would simply allow you to chain together test methods using @Next; which is pretty much the minimum needed to support scenarios. One benefit here is that nothing else changes and all the other planned changes (e.g. dependency injection via parameters) and annotations should work here for scenarios as well.

bechte commented 8 years ago

Three things I would like to add to the discussion:

1) Of course we could simply use @Test on the class level to minimize the number of annotations that JUnit serves. But I don't see a good reason for doing so. We are not aiming on to minimize the number of annotations, but we are maximizing the flexibility and the expressiveness of JUnit itself. In this manner, I think it is a good thing to introduce a separate annotation that clarifies that we are testing scenarios, i.e. @ScenarioTest. I really think, this is a good thing. Anyways, we still could change the name to something else (even though I like it the way it is).

2) We could introduce another engine that supports scenarios but this also means that this engine is required to provide its own semantics (probably using its own annotations).

3) The @Step annotation with a next attribute is an early draft. A @Next annotation would also solve this issue of finding out what the next step is in the scenario. Still, we should also think about some form of preconditions. I prefer specifying previous steps, i.e. what are the preconditions for this actual step to run? In this way, we don't get a list of steps, but a tree of steps, which I think is more flexible. But we might find it to hard to solve the problem internally and stick by the list approach. Just to mention...

I really think we should take this issue and have a group discussion within the dev team in January.

eric-thelin commented 8 years ago

Hi there! I enjoy reading the discussion so far. One idea that came to mind was allowing to define the step ordering in a single class-level annotation like so:

@Scenario(steps=["login", "doSomething", "logout"])
class ScenarioExample {

    @Step
    void login() {...}

    @Step
    void doSomething() {...}

    @Step
    void logout() {...}
}

Besides making it easy to see the step ordering, something I think is hard with the approaches referring to the next/previous step only, it would be possible to support definition of multiple scenarios on the same class:

@Scenario(name="Something", steps=["login", "doSomething", "logout"])
@Scenario(name="Something else", steps=["login", "doSomethingElse", "logout"])
class MultipleScenariosExample {

    @Step
    void login() {...}

    @Step
    void doSomething() {...}

    @Step
    void doSomethingElse() {...}

    @Step
    void logout() {...}
}

Thoughts?

pgpx commented 7 years ago

Sorry for maybe missing the point, but what is the main goal of scenario tests (just asking, not trying to be confrontational!)? Is it just to add steps, which effectively seem to be grouped assertions? Could you just make this a first-class concept and do something like (following on from the suggestion from @thomasdarimont):

@Test
void scenarioExample(TestInfo test) {
    test.step("login", () -> { ... });
    test.step("doSomething", () -> { ... });
    test.step("doSomethingElse", () -> { ... });
    test.step("logout", () -> { ... });
}

Or alternatively/in addition you could maintain a stack for a hierarchical assertion context:

@Test
void scenarioExample(TestInfo test) {
    test.pushStep("login");
    // ...
    test.step("doSomething");
    // ...
    test.step("doSomethingElse");
    // ...
    test.step("logout");
    // ...
}

This would involve a new concept of 'step' (which might well be out-of-scope), but I guess this is equivalent to the nested non-directly-runnable tests that this issue proposes?

Also, managing shared context across tests and controlling test order could also be important, but could potentially also be handled without having to change the semantics of instance-per-test/method (which other issues seem to suggest that this issue would solve, e.g. #48, #419). Maybe just passing around shared context objects (or parent test object instances)?

Ernesto-Maserati-CH commented 7 years ago

It seems that you will have too little time to design the "scenario testing" feature in a really good way. Then I would suggest to postpone it for a future JUnit version 5.1 maybe. I will maybe use Allure anyway, which already has scenario testing and other features around it. I suggest to take a look at how Allure solved this, also how it was solved in JBehave and Cucumber.

jillesvangurp commented 7 years ago

@Ernesto-Maserati-CH I think the semantics of scenario tests and how to fit them with junit is still a good topic for a major release like v5; especially because it is apparently hard/impossible currently with junit. Reading across the different issues (e.g. #48, #87) there are a lot of unresolved issues that all boil down to having more control over before and after semantics on things: in other words there is a problem with the semantics of the current set of annotations that do not allow for this. That sounds exactly like the kind of thing a major release needs to address.

There are a lot of legitimate concerns about backwards compatibility, limitations of the language (no predictable method order), the existing execution semantics that people are used to from past versions of junit, etc. However, I don't think there is actual broad consensus currently on how to address these topics or move forward on them; which I think was the point of this particular issue.

@pgpx the point about scenario tests is that they are very common in pretty much any type of test framework that derives from what ruby rspec and cucumber pioneered 10 years ago or so. It basically means that things execute in the order specified and can be nested and have predictable before/after semantics. It's useful for BDD style testing. It's useful for API/integration testing. Suffice to say that there are many valid reasons why you'd want to emulate that in Java on top of junit or testng.

@eric-thelin I like the proposed solution. You could argue that the @Step annotations are redundant (or at least optional) if you make the semantics of @Scenario that "every public method in this class is a step". I would prefer this because I kind of hate littering my code with annotations that state the obvious. Java is verbose enough as it is. As mentioned before, TestNg does this for @Test on classes.

In addition to step order, it would be nice to have a order=... attribute on the scenario as well. Valid values could be alphabetical,random,default (i.e. whatever junit does today). Unfortunately, the 'implied order' is not possible with Java. I think surefire provides a similar option.

I would argue the proposal also needs non static replacements for @BeforeClass and @AfterClass as well: how about @BeforeScenario @AfterScenario? Also we'd need BeforeStep and AfterStep. Probably out of scope but valid to consider would be concurrency and providing e.g. fork/join style semantics as well.

Another topic related to this is how to handle nesting of scenarios. It would be interesting to think about how this could work with nested classes so you can have nested scenarios as steps (each with their own before and after scenario behavior executing at the appropriate times. At least that is the closest I think we can get to rspec with this.

So putting it all together, something like this is what I'd actually like to be able to do:

@Scenario(order=ScenarioOrder.alphabetical, description="My first scenario")
public class MyScenario {

    @BeforeScenario
    public void beforeScenario() {
    }

    @BeforeStep
    public void before() {
    }

    public void step1() {
    }

    @Step(// custom options for this step tbd.)
    public void step2() {
    }

    @Scenario(order=ScenarioOrder.alphabetical, description="a nested scenario")
    public class Step3NestedScenario {
        @BeforeScenario
        public void beforeScenario() { // executes after the @AfterStep for step2 completes
        }

        public void step3_1() {
        }

        public void step3_2() {
        }        

        @AfterScenario
        public void afterScenario() {
        }
    }

    public void step4() {
    }

    @AfterStep
    public void afterStep() {        
    }

    @AfterScenario
    public void afterScenario() {
    }
}

I think, this proposal addresses most of the concerns and does not conflict with existing semantics; though it does raise a few interesting issues about what happens when you start mixing annotations, as people will no doubt attempt.

I've argued before in favor of fixing the semantics of the existing annotations before instead of adding new ones but from previous discussions with @sbrannen I understand that is not likely to get a lot of support so lets not go there again. Given that, a new set of annotations is the way forward.

The only other way I can think of would involve using lambda functions and varargs with some kind of builder pattern. However, I've yet to see a clean design for this in java though I've seen a few attempts and it would be hard to support with older versions of Java.

pgpx commented 7 years ago

@jillesvangurp I take your point, but I still don't really understand the need for steps vs just having ordered tests.

For example, in Cucumber you define your scenarios with Gherkin/feature files and only write your step definitions/matchers in Java (where JUnit annotations wouldn't be directly needed). I assume that the test runner would dynamically a single test for each scenario (or entry in a data table), ideally grouping assertions by step.

RSpec doesn't need the separate feature file, but (at least of the examples I've seen) scenarios are/could be essentially just methods containing further method calls for each step.

I guess I'm not sure why you would need/want each step to be a separate method (with state stored at a scenario level), and why before/after-step semantics are important (or couldn't be implemented with a simple lambda expression if needed).

I can see that complex integration tests might require a lot of setup that you don't want to repeat, but that seems to be a different concern that could maybe be handled differently (e.g. look at how Spring's integration test framework caches its own context), and those tests are not really separate steps in a single scenario.

I can also see that ordering between tests could be useful. For example you want to ensure that you run all tests related to setting up a database with static data before all of the tests that make use of that static data. However, this doesn't really seem to fit into a scenario (since a scenario implies a single test instead of multiple ordered tests), and is more like a test ordering that could be based on individual tests/test classes/tags (maybe like TestNG's dependsOnGroups and dependsOnMethods), though @dweiss's comments on #13 suggest a need for an explicit ordering anyway.

On the other hand, writing the @Scenario tests that you describe should be fine if you want to. I'm just not sure whether it would cover the cases I've just described, and might be better as an extension anyway?

panchenko commented 7 years ago

I would prefer the @Step annotation on the methods which also contains a number to specify execution order, as having method names in some annotation would make it more complicated to split or merge steps. Once I have implemented this approach as a Runner. An use case was integration tests doing some HTTP calls.

In junit5 it can be done quite easy like this:

public interface ScenarioSupport {
    @Retention(RetentionPolicy.RUNTIME)
    @Target(ElementType.METHOD)
    @interface Step {
        double value();
    }

    @TestFactory
    default Collection<DynamicTest> steps() {
        final List<Method> methods = Stream.of(getClass().getMethods())
                .filter(m -> !Modifier.isStatic(m.getModifiers()))
                .filter(m -> m.getParameterCount() == 0)
                .filter(m -> m.isAnnotationPresent(Step.class))
                .collect(Collectors.toList());
        methods.sort(Comparator.<Method>comparingDouble(m1 -> m1.getAnnotation(Step.class).value())
                .thenComparing(Method::getName));
        return methods.stream()
                .map(m -> dynamicTest(m.getName(), () -> m.invoke(this)))
                .collect(Collectors.toList());
    }
}

public class FooTest implements ScenarioSupport {
    private int counter;

    private int nextInt() {
        return ++counter;
    }

    @Step(1)
    public void stepA() {
        assertEquals(1, nextInt());
    }

    @Step(2)
    public void stepB() {
        assertEquals(2, nextInt());
    }

    @Step(3)
    public void stepC() {
        assertEquals(3, nextInt());
    }
}
sbrannen commented 7 years ago

@panchenko, that's certainly a clever use of default methods, reflection, and dynamic tests for implementing a feature not yet supported by the framework itself. 👍

Thanks for sharing!

panchenko commented 7 years ago

I am interested in implementing this properly in junit5, so would like to check if anyone is already actively working on it? And the next question is if we have some common vision on the approach?

marcphilipp commented 7 years ago

I don't know of anyone actively working on it. There's no common vision, yet. I think as a first step we need more detailed proposals how to express dependencies between steps each with a list of advantages/disadvantages, we need to define the semantics of lifecycle callbacks, e.g. is a @BeforeEach method invoked before each step etc.

sbrannen commented 7 years ago

I don't know of anyone actively working on it.

I've been working on it in my mind ... in case that counts. 😉

There's no common vision, yet.

True: there is no common vision, yet, but I think there is a general idea of how it could work.

For starters, #419 has to be resolved first. Second, even though #13 was closed quite a while ago, I believe a generic mechanism for ordering tests within a container will have to be in place before implementing support for scenario tests.

In other words, in order to remain true to the principles of JUnit, we must first have the necessary building blocks in place for any extension to implement something like scenario tests before the JUnit Team implements built-in support for scenario tests.

I think as a first step we need more detailed proposals how to express dependencies between steps each with a list of advantages/disadvantages,

I agree that this is the area that needs the most work.

we need to define the semantics of lifecycle callbacks, e.g. is a @BeforeEach method invoked before each step etc.

I've always been in favor of having them executed before each step.

panchenko commented 7 years ago

@sbrannen Sounds good, indeed solving these 2 parts would be enough here, I am going to start a PoC tomorrow.

sbrannen commented 7 years ago

@panchenko, I already solved and implemented the first one in the JUnit Lambda prototype.

So I'll most likely just reintroduce that support.

No need for a PoC, since it already exists. :wink:

smoyer64 commented 7 years ago

In many cases, explicit test ordering works great. There are other use cases where implicit ordering may be better:

  1. When testing UI user-stories, the successful path will often execute steps 1 through 10 in order, but there are multiple potential failure paths at each step. If I have steps 6a, 6b and 6c, only 6a would continue to step 7 and the others would expect some sort of failure response (a toast or alert on the UI?). Support for iterating through all the combinations of 1-5 then 6a, 6b and 6c would be great.

  2. A more abstract case is when you have a lot of small tests, potentially built up through test class inheritance. I would be interesting to be able to use the following code in this case:

@Test
@Predecessors({"testMethodOne", "testMethodTwo", "TestMethodX"})
void thisTestMethod() {... }

Clearly this is a lot more work as the test engine has to calculate the "test graph", verify there are no cycles and make sure there are no impossible constraints. I'd be interested to hear if there are others who think an extension point for controlling test method execution order would be useful. I don't think that either of these scenarios should really be built-in (though scenario one above is almost a test order + TestFactory mashup). This was one of my original motivations for #577 (Adds DiscoveryCallback). The parametric tests portion of that PR, description has clearly been addressed (and as noted, doesn't explode the size of the TestPlan the way #577 would have).

sbrannen commented 7 years ago

@smoyer64, some of your ideas might be achievable with something like a TestOrchestrator extension that I proposed here: https://github.com/junit-team/junit5/issues/14#issuecomment-264680160

smoyer64 commented 7 years ago

@sbrannen I had forgotten about that comment but I agree that it would support use-case #2 above. Use case #1 is harder as it requires a "fan-out" and if you need a clean context for each success/failure case in a step, we're back to needing to make extra instances. My intuition is saying we could accomplish use-case #1 by using @TestTemplate in a way that wasn't envisioned.

sbrannen commented 7 years ago

I had forgotten about that comment

No problem: I don't expect other people to remember all of the comments I've ever written. Heck, I don't even recall all of my own comments. 😉

My intuition is saying we could accomplish use-case #1 by using @TestTemplate in a way that wasn't envisioned.

Mmmm... possibly.

mfulton26 commented 7 years ago

It seems to me that the existing support for dynamic tests almost covers the needs for scenario tests:

    @TestFactory
    @DisplayName("Web Security Scenario")
    Stream<DynamicNode> scenarioTests() {
        return Stream.of(
                dynamicTest("Visit page requiring authorization while not logged in", () -> {
                    // attempt to visit page which requires that a user is logged in
                    // assert user is redirected to login page
                }),
                dynamicTest("Log-in", () -> {
                    // submit login form with valid credentials
                    // assert user is redirected back to previous page requiring authorization
                }),
                dynamicTest("Visit second page requiring authorization while logged in", () -> {
                    // visit another page which requires that a user is logged in
                    // assert user can access page
                }),
                dynamicTest("Log-out", () -> {
                    // visit logout URL
                    // assert user has been logged out
                })
        );
    }

The main drawback I see here other than the lack of injection of TestInfo, etc. is that if a dynamicTest fails the remaining tests are still executed instead of skipped/aborted due to a failed dependency.

What about the following?

    @TestFactory
    @DisplayName("Web Security Scenario")
    Stream<DynamicNode> scenarioTests() {
        return Stream.of(
                step("Visit page requiring authorization while not logged in", () -> {
                    // attempt to visit page which requires that a user is logged in
                    // assert user is redirected to login page
                }),
                step("Log-in", () -> {
                    // submit login form with valid credentials
                    // assert user is redirected back to previous page requiring authorization
                }),
                step("Visit second page requiring authorization while logged in", () -> {
                    // visit another page which requires that a user is logged in
                    // assert user can access page
                }),
                step("Log-out", () -> {
                    // visit logout URL
                    // assert user has been logged out
                })
        );
    }

Where step is a static method similar to dynamicTest but returns a DynamicNode implementation that signals this is a step and if a step fails the remaining steps should not be executed.

marcphilipp commented 7 years ago

Interesting idea! I think that might actually be more readable than using annotations. One thing that's missing is that you might want to re-use similar steps (e.g. the first two and the last one). So, instead of the third step we'd need something like alternativeSteps(step("Visit 2nd page", ...), step("Visit third page", ...), ...). Thoughts?

mfulton26 commented 7 years ago

Or just use dynamicContainer directly, right?

    @TestFactory
    @DisplayName("Web Security Scenario")
    Stream<DynamicNode> scenarioTests() {
        return Stream.of(
                step("Visit page requiring authorization while not logged in", () -> {
                    // attempt to visit page which requires that a user is logged in
                    // assert user is redirected to login page
                }),
                step("Log-in", () -> {
                    // submit login form with valid credentials
                    // assert user is redirected back to previous page requiring authorization
                }),
                dynamicContainer("Can access several pages while logged in", Stream.of(
                    step("Visit second page requiring authorization while logged in", () -> {
                        // visit another page which requires that a user is logged in
                        // assert user can access page
                    }),
                    step("Visit third page requiring authorization while logged in", () -> {
                        // visit another page which requires that a user is logged in
                        // assert user can access page
                    })
                )),
                step("Log-out", () -> {
                    // visit logout URL
                    // assert user has been logged out
                })
        );
    }

With that in mind it might make more sense to name a step "dynamicStep" and then whenever test developers don't care what order tests are executed in and there is no execution dependency they can use "dynamicTest" too. e.g.:

    @TestFactory
    @DisplayName("Web Security Scenario")
    Stream<DynamicNode> scenarioTests() {
        return Stream.of(
                dynamicStep("Visit page requiring authorization while not logged in", () -> {
                    // attempt to visit page which requires that a user is logged in
                    // assert user is redirected to login page
                }),
                dynamicStep("Log-in", () -> {
                    // submit login form with valid credentials
                    // assert user is redirected back to previous page requiring authorization
                }),
                dynamicContainer("Can access several pages while logged in", Stream.of(

                    // note: ordering of pages visited within this container doesn't matter

                    dynamicTest("Visit second page requiring authorization while logged in", () -> {
                        // visit another page which requires that a user is logged in
                        // assert user can access page
                    }),
                    dynamicTest("Visit third page requiring authorization while logged in", () -> {
                        // visit another page which requires that a user is logged in
                        // assert user can access page
                    })
                )),
                dynamicStep("Log-out", () -> {
                    // visit logout URL
                    // assert user has been logged out
                })
        );
    }
sormuras commented 7 years ago

Mh, what about an optional flag passed to @TestFactory allowing to alter the "fail fast" or "assert all" mode? Then dynamicContainer and dynamicTest can stay as they are.

@TestFactory(RunMode.ASSERT_ALL) -- like today

@TestFactory(RunMode.FAIL_FAST) -- exit when an error occurs and mark remaining nodes as "skipped"

mfulton26 commented 7 years ago

I like that. Another idea I just had would be to introduce DynamicScenario in addition to a DynamicStep. A "Scenario" is a "Container" like a "Step" is a "Test" but the prior have dependencies while the later do not. e.g.:

    @TestFactory
    @DisplayName("Web Security Scenario")
    Stream<DynamicNode> scenarioTests() {
        return Stream.of(
                dynamicStep("Visit page requiring authorization while not logged in", () -> {
                    // attempt to visit page which requires that a user is logged in
                    // assert user is redirected to login page
                }),
                dynamicStep("Log-in", () -> {
                    // submit login form with valid credentials
                    // assert user is redirected back to previous page requiring authorization
                }),
                dynamicScenario("Can access several pages while logged in", Stream.of(

                    // note: ordering of pages visited within this container doesn't matter

                    dynamicTest("Visit second page requiring authorization while logged in", () -> {
                        // visit another page which requires that a user is logged in
                        // assert user can access page
                    }),
                    dynamicTest("Visit third page requiring authorization while logged in", () -> {
                        // visit another page which requires that a user is logged in
                        // assert user can access page
                    })
                )),
                dynamicStep("Log-out", () -> {
                    // visit logout URL
                    // assert user has been logged out
                })
        );
    }
sormuras commented 7 years ago

I had some fun to implement a variation of my suggestion above. Assume your example scenario outputs:

  | '-- scenarioTests() [OK]
  |   +-- Visit page requiring authorization while not logged in [OK]
  |   +-- Log-in [OK]
  |   +-- Can access several pages while logged in [OK]
  |   | +-- Visit second page requiring authorization while logged in [OK]
  |   | '-- Visit third page requiring authorization while logged in [OK]
  |   '-- Log-out [OK]

Now, insert some failing check, like:

        dynamicTest("Log-in", () -> {
          fail("you shall not pass");
        }),

...and it takes the short way out:

  | +-- scenarioTests() [OK]
  | | +-- Visit page requiring authorization while not logged in [OK]
  | | '-- Log-in [X] you shall not pass

See https://github.com/junit-team/junit5/compare/issues/48-testfactory_failfast for an initial draft.

sormuras commented 7 years ago

Meta-annotated Scenario annotation added: https://github.com/junit-team/junit5/commit/9b1f57be9f2f97209b04e22e65c1f3555824c64c ... looks nice, but feels like an overkill.

mfulton26 commented 7 years ago

Neat.

One of the reasons I was thinking of being able to mix tests with steps (and containers with scenarios) was so that you can specify nonblocking tests (or containers) so that during a scenario you could specify some tests/containers that if they failed they wouldn't block execution of the remaining steps/scenarios.

As such, I do not believe that a single Scenario annotation is sufficient but a way to mix and match.

sormuras commented 7 years ago

dynamicStep introduced. That simplified the implementation as well.

I left dynamicScenario out on purpose, because a container can not produce an execution result. If one its blocking nodes fails, the container fails too. If all nodes are executed successfully, it is marked successful too.

jillesvangurp commented 7 years ago

@sormuras awesome; I like where this is going. Just occured to me that it would be nice if methods like dynamicContainer could accept a varargs of DynamicTest...subTests. There are a lot of calls to Stream.of in the example.

jillesvangurp commented 7 years ago

Likewise, it would save a bit of boilerplate and nesting if you could make the testFactory an annotated field instead of a method:

@TestFactory
Stream<DynamicNode> scenarioTests = dynamicScenario("Web Security Scenario", ....);

Just a thought; this might violate some notions people have about statefulness.

sormuras commented 7 years ago

Good idea, @jillesvangurp -- already added via https://github.com/junit-team/junit5/commit/881a44495847b6c0db72fa119fa53cdc3a2b0da4

Hm, I don't like the "annotated field instead of a method" idea at all. Methods should stay the preferred entry points for Jupiter tests.