Add the posibility to use @Before with bindMany

[michelefi]

bindMany and All annotation doesn't permit to setup up the binded instance with a before annotated method.
The following code demonstrate the use case:

import org.jukito.All;
import org.jukito.JukitoModule;
import org.jukito.JukitoRunner;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;

interface TestInterface {
  void foo();
}
class ImplOne implements TestInterface {
  public void foo() {
  }
}

class ImplTwo implements TestInterface {
  public void foo() {
  }
}

@RunWith(JukitoRunner.class)
public class BindManyTest {

  public static class Module extends JukitoModule {

    @Override
    protected void configureTest() {
      bindMany(TestInterface.class, ImplOne.class, ImplTwo.class);
    }
  }

  @Before
  public void setup(TestInterface test) {
    System.out.println(test.getClass().getSimpleName());
  }

  @Test
  public void testGetFlight(@All TestInterface test) {
    System.out.println(test.getClass().getSimpleName());
  }
}

When running the test I expect as output:

ImplOne
ImplOne
ImplTwo
ImplTwo

Instead the output is:

TestInterface$$EnhancerByMockitoWithCGLIB$$78a15e3b
ImplOne
TestInterface$$EnhancerByMockitoWithCGLIB$$78a15e3b
ImplTwo

It's possible to support the All annotation also in Before method or fix the behavior?

[Tembrel]

I think it's going to be hard to define semantics for this that won't surprise people. What would happen in these cases?

@Before public void setup(@All Foo foo) { ... }
@Test public void test1(@All Foo foo) { ... }
@Test public void test2(@All Foo foo, @All Bar bar) { ... }
@Test public void test3(@All Bar bar) { ... }
@Test public void test4() { ... }

I don't think this enhancement is worth the confusion it would cause. The workaround I posted
here involves only a tiny amount of extra work and is easy to understand.

[michelefi]

I agree with you that add the @ALL annotation in the before method can
cause confusion, but create also confusion the fact that is injected a mock
if a @before or @after method id created.
Your workaround works but if it's possible to correct the behavior of the
injection on the @before method would be the best solution.

Il giorno gio 23 lug 2015 alle ore 16:01 Tim Peierls <
notifications@github.com> ha scritto:

I think it's going to be hard to define semantics for this that won't
surprise people. What would happen in these cases?

@before public void setup(@ALL Foo foo) { ... }
@test public void test1(@ALL Foo foo) { ... }
@test public void test2(@ALL Foo foo, @ALL Bar bar) { ... }
@test public void test3(@ALL Bar bar) { ... }
@test public void test4() { ... }

I don't think this enhancement is worth the confusion it would cause. The
workaround I posted
here https://gist.github.com/Tembrel/778a180412ef36f13572 involves only
a tiny amount of extra work and is easy to understand.

—
Reply to this email directly or view it on GitHub
#67 (comment).

[Tembrel]

Can you make a more specific proposal here? It's not enough to say "use @before with bindMany" -- you need to flesh out the semantics in detail.

[michelefi]

The proposal is to implement exact what you wrote in the Jukito forum:

The @All only works for parameters in @Test methods.
But you can add the same parameter (without @All) to the @Before method. This way the same instance is injected into both methods.

Now it does't work in that way but I think it's the correct behavior. If isn't possible for same technical issue we can use your workaround.

[Tembrel]

Those sentences (I didn't write them, I quoted them):

But you can add the same parameter (without @All) to the @Before method.
This way the same instance is injected into both methods.

They aren't a specification to be implemented. They don't, for example, say what should happen in this case:

@Before public void setup(Foo foo) {...}
@Test public void test1(@All Foo foo) {...}
@Test public void test2() {...}

And what would happen with multiple @ALL arguments in a test but only a subset of those types in a @before?

@Before public void setup(Foo foo) {...}
@Test public void test1(@All Foo foo, @All Bar bar) {...}
@Test public void test2(@All Foo foo) {...}
@Test public void test2(@All Bar bar) {...}

It's easy to describe what the @ALL does for test methods, but it gets complicated when describing how the @before method arguments would be injected.

I think it's better to be explicit. We're talking one line in each affected test method vs. a hard-to-specify new feature.

[dougmill]

I encountered and was annoyed by this absent feature a while back. If I remember the results of my investigation correctly, in Guice (and Jukito) each object to be injected is uniquely identified by the combination of the class and annotation(s) on its parameter declaration. The only reason Jukito doesn't already have this feature working is that @All gets transformed into something else before doing the Guice lookup in the @Test method invocation, and that same transformation is not applied for @Before or @After methods.

The appropriate specification seems very simple and not at all confusing to me: for each invocation of a @Test method, call each @Before and @After method once, injecting the same instance at every point where the class and annotations match in the original user-written code. The only point that strikes me as even mildly confusing is what to do when @Before calls for an @All parameter that the @Test method lacks.

Examples (of how I think it should work):
For each example, assume the module setup includes

bindManyInstances(Foo.class, foo1, foo2);
bindManyInstances(Bar.class, bar1, bar2);

@Before public void setup(Foo foo) {...}
@Test public void test(@All Foo foo) {...}

Call setup with an automatically generated mock object because the annotations don't match, then call test with foo1. Call setup with a second automatically generated mock object, then call test with foo2. This is how Jukito currently behaves and I wouldn't change it for this case.

@Before public void setup(Foo foo) {...}
@Test public void test(@All Foo foo, Foo foo3) {...}

Call setup with an automatically generated mock object, then call test with foo1 for the first parameter and the mock generated for setup as the second parameter. Importantly, foo in setup and foo3 in test are the same instance. Repeat with foo2 and another generated mock. This is again how Jukito currently behaves and should not be changed. In fact, this case and preserving its proper functionality is a major reason why the first example should not be changed.

@Before public void setup(@All Foo foo) {...}
@Test public void test(@All Foo foo) {...}

Call setup with foo1, then test with foo1, then setup with foo2, then test with foo2. Jukito currently handles this the same as the first example. I traced the code in the debugger a while back, the lookup performed for injection into setup has foo1/foo2 (I think only one of them at a time, but I'm not certain about my memory on that point) available in the map but skips them because the annotations recorded in the map don't match @All.

@Before public void setup(@All Foo foo) {...}
@Test public void test(@All Foo foo, @All Bar bar) {...}

Call:
setup(foo1), test(foo1, bar1)
setup(foo1), test(foo1, bar2)
setup(foo2), test(foo2, bar1)
setup(foo2), test(foo2, bar2)

@Before public void setup(@All Foo foo) {...}
@Test public void test() {...}

Call setup with either foo1 or foo2, then call test with its empty parameter list. Declare in documentation that which of the many bound instances gets passed to setup in this case is unspecified.

@Before public void setup(@All Foo foo) {...}
@Test public void test(@All Bar bar) {...}

Call setup with either foo1 or foo2 (unspecified), then test with bar1, then setup with foo1 or foo2 again, then test with bar2.

@Before public void setup(@All Foo foo) {...}
@Test public void test(Foo foo) {...}

Call setup with either foo1 or foo2 (unspecified), then test with an automatically generated mock because it's not using @All and the only Foo binding is a bindMany.

[Tembrel]

Thanks for starting to spec this out. My feeling is still that the definitional complexity outweighs the marginal benefit, but if someone is willing to complete the spec and implement it, I don't see that it does any harm. (Yes, I'm damning with faint praise.)

Does no one else see what I mean by definitional complexity? I would have trouble using this feature, because I wouldn't know when to add @All and when not to, except by looking it up in the documentation. Even knowing how it worked, I'd probably end up just writing an unannotated helper function to be used by test methods with @All arguments, simply to avoid the risk that someone reading my test methods later would be confused.

[dougmill]

When to add @All and when not to in a @Before method is simple: add it if and only if you want the instance that a @Test method using @All will receive. I fail to see how there is any difficult to understand complexity in that principle.

Maybe you're getting confused by thoughts of how @All would affect how many times @Before gets called? If so, just realize that there would be no effect whatsoever on that. In setup methods, @All would serve only as an identifier with no special behavior attached. It already gets run once per combination of @All bindings because that's how many times the test methods get run and @Before is run once per test method invocation.

[Tembrel]

No, I did understand what you wrote. I'm not saying it's impossible to grok. I'm saying that someone coming to this fresh might be excused for being unsure of whether the @All is needed/allowed in @Before methods, and what it means if omitted.

Putting myself in the head of such a developer, I expect the price of this uncertainty would lead me just to write explicitly what I wanted rather than trusting my instincts or reaching for the docs.