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

@Rule and @ClassRule support #49

Closed
rkawala opened this issue Nov 18, 2016 · 19 comments
Closed

@Rule and @ClassRule support #49

rkawala opened this issue Nov 18, 2016 · 19 comments

Comments

@rkawala
Copy link

rkawala commented Nov 18, 2016

JUnit provides @ClassRule and @rule annotations, which would let me use the new @SpringBootTest annotation with Spectrum. Is there any plan to provide support for @ClassRule and @rule? Thanks!

@greghaskins
Copy link
Owner

I like the idea of using existing JUnit extension points like @Rule and @ClassRule if we can. The tricky bit will be figuring out a clean way to do so. Most of the Spring testing stuff relies on custom subclasses of the base JUnit runner, which would be difficult to integrate directly into Spectrum unless we copy their code.

For example, based on my reading of the Spring docs, using @SpringBootTest requires using their custom runner (@RunWith(SpringRunner.class)) and thus you couldn't use @RunWith(Spectrum.class).

@greghaskins
Copy link
Owner

I don't think extending ParentRunner is a viable option for Spectrum. Its implementation makes some fundamental assumptions that conflict with Spectrum, like tests being declared as methods, suites being simple Java classes etc.

However, it would be fairly straightforward to add an adapter so that we could use Rules with Spectrum, just declared in a different way. Borrowing some examples from the JUnit wiki it could look something like this:

describe("a possible way of integrating JUnit rules", () -> {

    Supplier<TemporaryFolder> tempFolder = rule(() -> new TemporaryFolder());

    Supplier<ExpectedException> exception = rule(() -> ExpectedException.none());

    it("counts assets", () -> {
        File icon = tempFolder.get().newFile("icon.png");
        File assets = tempFolder.get().newFolder("assets");
        createAssets(assets, 3);

        DigitalAssetManager dam = new DigitalAssetManager(icon, assets);
        assertEquals(3, dam.getAssetCount());
    });

    it("throws IllegalArgumentException if icon is null", () -> {
        exception.get().expect(IllegalArgumentException.class);
        exception.get().expectMessage("Icon is null, not a file, or doesn't exist.");
        new DigitalAssetManager(null, null);
    });

});

I still don't think this will make Spring stuff "just work" but it would provide the beginnings of some extension points.

Thoughts?

@greghaskins
Copy link
Owner

@ashleyfrieze the above ideas might be useful for you as well.

@ashleyfrieze
Copy link
Contributor

For this to work with Spring, it would need to be some @rule objects within the test class e.g.

@RunWith(Spectrum.class)
public class SpectrumTest {
    @Rule
    public void SomeSpringRule rule = ...;

    {
        describe("Something awesome", () -> {
             // stuff goes in here
        });
    }

I nearly got it working using subclasses of Spectrum, but came to the conclusion that there's just one or two more extension points needed hence my fork.

@greghaskins
Copy link
Owner

greghaskins commented Nov 25, 2016

Excellent.

In terms of how to use Rules with Spectrum, my preference would be a functional style similar to the above, if possible. With a field annotation, you lose lose flexibility with nesting for example. I like to think of specs as essentially pure function closures, not tied to a particular object instance. In the example I gave, the Rule objects kind of mix in with some of the let() functionality.

What do you think?

@ashleyfrieze
Copy link
Contributor

I think it won't work because JUnit doesn't work that way.

I think we will have to accept rules the way they are. They are expected to be on the test class. I am going to suggest that for the purposes of doing this, that each ClassRule will be applied to the whole test class, and each method rule be applied to the whole top level "describe".

@ashleyfrieze
Copy link
Contributor

FWIW - you can see some changes appearing here:
https://github.com/ashleyfrieze/spectrum/tree/af-bdd

@greghaskins
Copy link
Owner

Fair enough, you may be right. I still want to believe there's a way to make it work, but will need to sit down to figure that out.

I like where you're going with given/when/then. Let's do that as one PR, and tackle the Rule/ClassRule with another one. I created #50 to track that separately from this thread.

@ashleyfrieze
Copy link
Contributor

When you look at how the rules actually work, and how Spring even validates that they're being used correctly... well, it has to be the way that JUnit intended it. There may be something better when JUnit 5 comes along.

Please see the second commit on my scratch branch. I've got rules working. It kinda sucks for now. I'll split them into two PRs as you say - just cranking out some ideas at this point.

Feedback appreciated while the code is hot.

What I'm trying to do is sneak in a way to parallelise at the suite level. It's not doing it yet, but the fact that there are two aspects of:

  1. Method rule
  2. Parallel

It might do it...

But...

I think it might not be able to, since the steps cannot really reference the object around them if it's "newed" multiple times, so there'd be no thread safety if we parallelised.

@greghaskins
Copy link
Owner

I checked out the spring-test code--you're right, it is very coupled to the way "normal" JUnit works with field reflection and re-instantiated objects. This is essentially in conflict with Spectrum's Block-based functional implementation. Using fields in Spectrum leads to weird behavior between tests and moreover breaks the core contract about nesting: that you can do the same things at any level of nesting. (Fields only apply to the top-most level, and further don't get re-initialized between tests.)

The spring-test library has different helper implementations for JUnit4, TestNG, JUnit5, etc. for exactly this reason: they all have different APIs and assumptions.

If the goal is to make testing Spring stuff easier for Spectrum users, then we should build that functionality directly in a package like spectrum-spring that depends on spring-test and provides 100% compatibility with the Spectrum API. We can probably leverage a good bit of the helper code in spring-test that is generic across test runners. I would be willing to write such a library myself or collaborate with the community to make it happen.

As an aside for #15, I think the same thing applies for Mockito. We should create a spectrum-mockito rather than tying ourselves even more closely to JUnit4-specific implementation details.

@ashleyfrieze
Copy link
Contributor

I had a way to give unsurprising behavior with the local fields, rules, and levels of test.

I'll share a POC if you are interested.

The main point is that each spec runs with access to the parent object and any fields it references are evaluated at the point of execution not at the point of test definition.

@ashleyfrieze
Copy link
Contributor

ashleyfrieze commented Nov 28, 2016

For the current latest ideas of user experience of the JUnit class rule support see posts on #15

@rkawala
Copy link
Author

rkawala commented Nov 28, 2016

BTW, I was able to get Spring working with Spectrum. I just create my own instance of TestContextManager and everything works. But it sure would be cool to be able to use @ClassRule and @rule. One question: given that JUnit expects to have a different instance of the test class for each test, and Spectrum creates one instance for all the tests (like TestNG does, I believe), would @rule work the same way it does in JUnit?

@greghaskins
Copy link
Owner

Good question @rkawala. If JUnit rules expect a fresh instance for each test case, then there may be some subtle bugs that pop up when we re-use the instance between specs.

I think our current line of thinking (from this sketch) is that we'd use a helper class that contains all the JUnit annotations and rules, and that would get a fresh instance for each spec (presumably @ashleyfrieze ?).

@ashleyfrieze
Copy link
Contributor

With the proposal in #15, I suggested that it shoudl be possible for the class with the @rule wiring in to be declared separately and newed every time. E.g.

@RunWith(Spectrum.class)
class MyTests {{
    // where Context is full of @Rule and @Autowired and other annotations used by SpringJUnit
    Supplier<Context> context = rule(Context.class); 

    describe("some suite", () -> {
        it("can  use the context which is fresh here", () -> {
            context.get().getAutowiredThing();
        });

        it("can reuse that thing here because it's not fresh, since the rule is in the parent scope", () -> {});
    });

    describe("and this suite", () -> {
         it("gets a new one to play with", () -> {});
    });
}}

@ashleyfrieze
Copy link
Contributor

My hope is to allow a permissive user to ALSO use the parent class of the tests to do JUnit wiring, which seems to work fine for simple things like TestFolder or Mockito.

@ashleyfrieze
Copy link
Contributor

@rkawala - would you like to comment on #56 - see the new example specs and how rules are shaping up.

@ashleyfrieze
Copy link
Contributor

@rkawala - #56 is now quite a long way along - would it solve your problem?

@greghaskins
Copy link
Owner

This is mostly resolved by #56. Closing unless we determine otherwise.

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

No branches or pull requests

3 participants