Support multiple bindings of GuiceBerryEnvMain

[GoogleCodeExporter]

It should be possible to write tests using Guiceberry testing infrastructure 
that is contributed by different components/teams in a system. 

Only one of GuiceBerryEnvMain can be bound per Injector, which means if 
ComponentA testing infrastructure and ComponentB testing infrastructure both 
need to start a server, they can't both use this mechanism. 

Work around:-

Each project could move the bind(GuiceBerryEnvMain.class).to(...) into its 
TopLevelEnv module, but then each project that requires to start more than one 
server requires a custom GuiceBerryEnvMain which starts all of the dependent 
servers, maybe by injecting each concrete implementation of GuiceBerryEnvMain: -

@Inject
ProjectAGuiceBerryEnvMain(ProjectAServerStarter a, ProjectBGuiceBerryEnvMain b, 
ProjectCGuiceBerryEnvMain c) {
  a.start();
  b.start();
  c.start()
}

This may still be needed where finer control is required, e.g: servers started 
in a particular order, or custom logic based on failures of one component, but 
I feel the majority of projects could benefit from a simpler, out of the box 
solution.

Proposal:-

In GuiceBerryUniverse.callGbeMainIfBound() add support for multi bound 
GuiceBerryEnvMain's. Each GuiceBerryEnvMain could be started in parallel, with 
total failure if any one of them fail to start.

1) What to do about projects that have an existing binding for 
GuiceBerryEnvMain?

Both the com.google.guiceberry and com.google.inject.testing.guiceberry 
(deprecated version) could be started first independently if bound.

2) Multibindings do not have a clean and simple API.

Personally I find having to call Multibinder.newSetBinder().addBinding() a 
little ugly. Do we care about this, or is it better to add a cleaner method in 
GuiceBerryModule that subclasses can call

addGuiceBerryEnvMainBinding().... (returns 
LinkedBindingBuilder<GuiceBerryModule>) which would hide the multibinding 
implementation behind a cleaner API.

Thoughts?

Original issue reported on code.google.com by jongerr...@google.com on 8 Feb 2012 at 8:18

[GoogleCodeExporter]

(spolier alert: good news below)

I've been struggling since you first suggested this feature between seeing the 
usefulness, and being antsy about the "lock in nature" (e.g. whichever of 
"start in parallel" or "start in sequence" that we may choose, we'll make some 
people discontent), not to mention the funny thing about the different 
semantics of GBEMain between projects that use or not multibindings.

So I was pleased with an idea that popped up in my head, and I think satisfies 
your needs (which I agree are general) and does not have the drawbacks 
aforementioned.

My idea is (all names here are quite tentative, and not thought out, I'd love 
better suggestions):

a. leave GBEMain semantics exactly as they are

b. create an "interface GuiceBerryEnvMainlet" with a "void run()" method

c. create a "class ParallelGuiceBerryEnvMainletStarter implements 
GuiceBerryEnvMain" that expects multi-bound GBEMainlets (I'm thinking a List 
rather than Set)

d. with that, someone is free to either: 1. choose to bind GBEMain to 
ParallelGBEMainletStarter, and leverage the multibindings; 2. use their own 
GBEMain, as they do now, without honoring the Mainlet multibindings at all; 3. 
create their own GBEMainletStarter thingy that honors the Mainlet multibindings 
in any way they choose; 4. mix and match all of these

e. we might as well also have a SequentialGBEMainletStarter

Of course, all this can be done even without modifying GuiceBerry proper, but 
I'd be more than happy to have these classes/interfaces in GB itself, so as to 
encourage the pattern.

Does that satisfy your need, or am I missing something?

Z

Original comment by zorze...@gmail.com on 13 Feb 2012 at 1:38

[GoogleCodeExporter]

That would satisfy my needs perfectly. However I'd like to raise to points and 
play devils advocate if I may ;-)

1) I'm worried that adding a the GuiceBerryEnvMainlet infrastructure would 
clutter the GuiceBerry Api... there is now the GuiceBerryEnv standard way to 
start a server, then this extension (and if you want to get rid of it later it 
may be harder)... it could be put into a .contrib or .ext package and/or be an 
optional jar.. points worth considering at least. What do you think?

2) Whether GuiceBerryEnvMainlets are multi bound, or if GuiceBerryEnv's 
themselves could be multi bound, there is still an awkwardness of the 
multibinding api to consider.... having various places in the code base 
contribute to the multi binding always feels to loose for an API in my 
opinion.... my preference would be to create a dedicated module for this

install(new GuiceBerryEnvMainModule(MyGuiceBerryEnv.class));

or mainlet if you want to go that way?

What do you think? I'm happy to submit a patch either way :-)

Original comment by jongerr...@google.com on 13 Mar 2012 at 2:24

[GoogleCodeExporter]

> 1) I'm worried that adding a the GuiceBerryEnvMainlet infrastructure would 
clutter
> the GuiceBerry Api... 

Clutter is a legit concern, but the feature is compelling (and the clutter 
small), and I'm happy to add the concept as an "advanced" collection of classes 
-- just like controllable injections.

> there is now the GuiceBerryEnv standard way to start a server

which would continue existing exactly as today (one reason I like this 
approach).

> then this extension (and if you want to get rid of it later it may be 
harder)... 

extension == useful classes that someone else could have done outside of GB 
proper, that I chose to feature, by bundling

harder == not going to happen. I'm ok with that

> it could be put into a .contrib or .ext package and/or be an optional jar.. 
points worth considering at
> least. What do you think?

Separate java package (no need for a separate jar), just for this stuff plus 
#23. How about "components" for a package name? We could name these 
GuiceBerryMainComponent and TestWrapperComponent

> there is still an awkwardness of the multibinding api to consider.... having 
various places in the
> code base contribute to the multi binding always feels to loose for an API in 
my opinion.... 
> my preference would be to create a dedicated module for this

I don't fully understand how it would look like with a "dedicated module", but 
I think all we need is a "GuiceBerryComponets" class with a static method for 
adding an entry to the (internally multi-bound) "List<GBMC>" (and another for 
the test wrapper thing).

I look forward to patches.

Be warned: 95% of this change will be docs (in the form of javadoc, tutorial 
classes, tutorial doc and announcement).

Zorzella

Original comment by zorze...@gmail.com on 13 Mar 2012 at 5:58

[GoogleCodeExporter]

Thanks, this sounds good to me and I think I have enough info to start working 
on some patches.

Original comment by jongerr...@google.com on 13 Mar 2012 at 6:02

[GoogleCodeExporter]

Hi,

Included is a patch which contains TestWrapperModule, installing this and 
calling the associated static method allows for installing multiple 
TestWrappers. Included a test.

Issue #23 is very similar, so I will await your feedback on this to make sure 
it is in the general direction you're happy with before making a start.

Original comment by jongerr...@google.com on 13 Mar 2012 at 11:43

Attachments:

• MULTIPLE_TESTWAPPER_MODULE.patch

[GoogleCodeExporter]

My apologies for taking this long to review this.

There are a few things I like and a few I dislike about the patch:

* if I get this right, you decided to, instead of creating a new 
TestWrapperlet, or TestWrapperComponent or whatever, leverage a binding 
annotation. I must confess that at first look I disliked the idea, but the more 
I think about it, the more I like it. So you leverage a binding annotation, so 
as to (for example) allow you to choose to use a TestWrapper someone else wrote 
(originally as a full wrapper), and instead multibind it. It has the added 
benefit of not creating a new interface that looks exactly like TestWrapper. 
The only downside I can see is that it makes the intent of TestWrapper more 
complicated. I need to sleep over this, but I'm reasonably convinced I the pros 
win handily over the cons. Your patch will need, though, to include changes 
(additions) to the TestWrapper javadoc.

* We want to use List rather than Set, so the iteration order is 
predictable/stable.

* I don't like the name CompositeTestWrapper. Something like 
SequentialTestWrapperExecutor feels right, particularly because...

* ... we should provide for a ParallelTestWrapperExecutor counterpart right off 
the bet

* Both of these *Executors should be public

* I don't think that TestWrapperModule is useful. Other than the static method 
(see below), it only adds a binding that IMHO should just be done in people's 
GB modules.

* I think the static multibind method should be in, say, a GuiceBerryComponents 
class with just static methods. I think it even makes sense that the same class 
have both multibind methods for TWrapper and GBEMain

* @Internal should be public -- so as, e.g., to allow for someone to write 
their own MyNotQuiteParallelTestWrapperExecutor, if they so wish, and simply 
replace one executor for another. Not that I think this will be done, but I 
feel the walled garden alternative here is not really beneficial in a 
significant way.

* @Internal should be named something like @Multibound (and I think we should 
use the same binding annotation for both TestWrapper and GBEMain)

* I'm not familiar with using "@Qualifier @Documented @Retention(RUNTIME)" to 
create a binding annotation. It's not how it's done elsewhere in GuiceBerry nor 
I see anything in BindingAnnotation's javadoc about there being a different way 
of doing it. Do you have a particular reason to use this construct? Any link 
with info about this?

* this needs lots of docs: particularly javadoc for all the public classes and 
methods, and a a new tutorial for this.

BTW, despite the nitpicking, I really like the idea and I'm happy with what I 
think this will look like!!!

Original comment by zorze...@gmail.com on 24 Mar 2012 at 7:27

[GoogleCodeExporter]

Find attached a new patch, and I've made a slight change in direction in 
response to your comments, its still pretty rough but if you are happy with the 
direction I can start to add some polish to the patch :-) I'll address you 
comments below:-

* Guice Multibindings don't support List, BUT the Set is iterable in the order 
the components were bound, although its recommended not to rely on ordering as 
you can be relying on the order of module installation, you can read more about 
it here:-

http://google-guice.googlecode.com/svn/trunk/latest-javadoc/com/google/inject/mu
ltibindings/Multibinder.html

* I changed the name from CompositeTestWrapper to SequentialTestWrapperExecutor 
as requested and also added a ParallelTestWrapperExecutor (they share all the 
same code with the variation being the ExecutorService implementation used. If 
you don't like having the two sub classes with different constructor 
configurations an alternative is to remove them, but require the user to bind 
an ExecutorService manually. Perhaps there could be an @GuiceBerryComponents 
annotation so it doesn't clash with other ExecutorServices bound.... personally 
I prefer the subclasses though.

* I made both Executors public

* I added a similar implementation for *GuiceBerryEnvMainExecutor

* I removed the @Internal annotation, its purpose was to be package private for 
the purpose of the multibindings, in order to create that walled garden 
implementation, so all bindings are hidden by the static method, and would not 
clash with other bindings of TestWrapper/GuiceBerryEnvMain elsewhere in the 
system. I think you are probably right though, the walled garden approach is a 
waste of time, since these interfaces are very specific to GuiceBerry and any 
bindings of them will be in the context of GuiceBerry.

* I removed the TestWrapperModule. I removed the static method too, I'm not 
sure how helpful it is now.

* I added a new tutorial class, I hope its in the right location, it feels more 
like an integration test though, but if you have any guidance on this I'd 
appreciate it... should I add some text for the tutorial, is this just on 
Google Docs?

If you are happy with the direction, let me know and I'll go through and add 
more docs etc.

Original comment by jongerr...@google.com on 25 Mar 2012 at 6:23

Attachments:

• guiceberry_components_v2_patch.patch

[GoogleCodeExporter]

I'm very happy with the process:

*********************************

* Guice Multibindings don't support List, BUT the Set is iterable in the order 
the components were bound

Ok

* I changed the name from CompositeTestWrapper to SequentialTestWrapperExecutor 
as requested and also added a ParallelTestWrapperExecutor

excellent. ParallelGuiceBerryEnvMain lacks the "Executor" suffix.

> (they share all the same code with the variation being the ExecutorService

I love it -- this is excellent! Can't you make them delegate rather than 
subclass, so as to make the "base class" final? Also, I don't quite understand 
(and it's not javadoc'ed) the semantics of what happens when one or more 
exceptions are thrown from the multibound TWs/GBEMs.

* I made both Executors public

Ok.

* I added a similar implementation for *GuiceBerryEnvMainExecutor

Ok.

* I removed the @Internal annotation, its purpose was to be package private for 
the purpose of the multibindings

I told you I'd sleep on using the annotated TWrapper/GBEMain instead of 
creating new classes. I'm fine with that. Note, though, that this will likely 
be a little less of a panacea than I first anticipated, since it's much more 
common than not to have TWs/GBEMs be @Provides methods, which are not 
immediately transformable into multibindings. Nevertheless, I'm still fine with 
the usage.

I do have to sleep over the idea of not using an annotation, which is a minor 
point. I'll let you know.

* I removed the TestWrapperModule. I removed the static method too, I'm not 
sure how helpful it is now.

The static method still seems useful, particularly wrt to documentation and 
usage findability. I was also thinking it can also make syntax terser with 
varargs, i.e. a signature like:

static void multiBindTestWrapper(Binder, TestWrapper...)

* I added a new tutorial class, I hope its in the right location, it feels more 
like an integration test though, but if you have any guidance on this I'd 
appreciate it... should I add some text for the tutorial, is this just on 
Google Docs?

Yes, the tutorials end up being the simplest integration tests of the features 
(clearest e.g. controllable injections). It should be in the 
'junit4/tutorial_0_basic' tree (each tutorial is given in the 4 testing tools 
supported -- we'll leave this for last; it's a bit tedious, but not rather 
simple to do). Also put a .txt as part of the patch that I'll not submit, but 
rather I'll include in the Google Docs tutorial doc.

> If you are happy with the direction, let me know and I'll go through and add 
more docs etc.

Very happy. We only need more doc and some polish.

Oh, but one last thing: I'd rather not add a dep to mockito (or anything else), 
unless there's a pretty good reason...

Original comment by zorze...@gmail.com on 26 Mar 2012 at 4:58

[GoogleCodeExporter]

* ParallelGuiceBerryEnvMain lacks the "Executor" suffix.

-- Done

* Can't you make them delegate rather than subclass, so as to make the "base 
class" final? 

-- I could, but I think this would cause some other awkwardness in the code, 
namely that:-

* If we delegate we'd have to either inject the TestWrapperExecutor or just 
'new' it. If we inject it then we'd need to bind ExecutorService somewhere and 
with an annotation (because I don't think we should bind a global 
ExecutorService since its probably a very common binding that may clash), and 
then we'd need to control which executor service gets bound and there could be 
four possible scenarios Parallel/Sequential Vs TestWrapper/GuiceBerryEnvMain. 
'new'ing it feels awkward too, we'd need to create the delegate within each 
concrete class, save it as a member and override the run method which is more 
boiler plate. What would you like to achieve by making 
{GuiceBerryEnvMain,TestWrapper} final? Are these classes you'd like to expose 
in the API? If not, we can make the classes package private, then only the 
XXSequentialExecutor and XXParallelExecutor are part of the public API, then 
the implementation details (extension vs composition matter less). Another 
option which I like is to have a GuiceBerryComponents class of static methods 
which could install the bindings on the users behalf e.g:

public class GuiceBerryComponents {

  private GuiceBerryComponents() { // Utility methods, non-instantiable }

  public Module newParallelGuiceBerryMainModule() {
    return new AbstractModule() {
      protected void configure() }
        bind(GuiceBerryEnvMain.class).to(GuiceBerryEnvMainExecutor.class);
        bind(ExecutorService.class).annotatedWith(GuiceBerryEnvMainExecutor.class).toInstance(Executors.newCachedThreadPool());
      }
    }
  }
} 

There would be similar methods for Sequential and corresponding ones for 
TestWrapperExecutor too. The advantage of this approach is that it would remove 
the four boiler plate concrete classes. It might be possible to replace the 
custom annotation (or @Named) for the executor service with PrivateModules and 
expose() but I'd need to check if that works with the multi bindings. 
GuiceBerryComponents would also be the class to have the helper methods for 
adding the multi bindings.

Yet another approach would be to go back to the Walled Garden idea of having a 
module that takes care of the internal bindings above but also exposes an API 
to support the multibindings.

The extension of package private base classes is the option I've taken in this 
patch as I'd rather not change too much unless I'm sure of the direction.

* I don't quite understand (and it's not javadoc'ed) the semantics of what 
happens when one or more exceptions are thrown from the multibound TWs/GBEMs.

I added javadoc for this onto the GuiceBerryEnvMainExecutor and 
TestWrapperExecutor… basically this follows the semantics of the 
ExecutorService, which when we call Future.get() to retrieve the void results, 
we'd be notified of an ExecutionException, and we'd end up propagating the 
first one. The alternative would be to catch and hold on to each 
ExecutionException and then throw some kind of UmbrellaException which contains 
all Exceptions that were caught.

* The static method still seems useful, particularly wrt to documentation and 
usage findability. I was also thinking it can also make syntax terser with 
varargs, i.e. a signature like:

static void multiBindTestWrapper(Binder, TestWrapper...)

We could go this way, but we'd end up copying a lot of the syntax of 
LinkedBindingBuilder, such as to(), toInstance(), toProvider() which we 
probably want to avoid.

Another approach would be to have this self documenting done using an abstract 
module, with the disadvantages that users need to subclass, but the advantages 
of a better syntax (callers are not passing binder() to the static methods).

public class GuiceBerryComponentsModule extends AbstractModule {

  protected LinkedBindingBuilder<GuiceBerryEnvMain> multiBindGuiceBerryEnvMain() {
    Multibinder<GuiceBerryEnvMain> envMainBinder = Multibinder.newSetBinder(binder(), GuiceBerryEnvMain.class);
    return envMainBinder.addBinding();
  }
}

client classes could then be written like:-

public class MyGuiceBerryComponentsModule extends GuiceBerryComponentsModule {

  protected void configure() {
    bind(TestWrapper.class).to(SequentialTestWrapper.class);
    multiBindTestWrapper().to(FirstTestWrapper.class);
    multiBindTestWrapper().toProvider(TestWrapperProvider.class);

    bind(GuiceBerryEnvMain.class).to(ParallelGuiceBerryEnvMain.class);
    mutliBindGuiceBerryEnvMain().toInstance(new MyEnvMain());
  }
}

* the tutorials end up being the simplest integration tests of the features 
(clearest e.g. controllable injections). It should be in the 
'junit4/tutorial_0_basic' tree

-- Done.

I'll get back to work on this when I receive your advice on some of the above 
questions, sorry that its taken so long to get around to it.

Original comment by jonat...@indiekid.org on 10 Apr 2012 at 5:16

Attachments:

• guiceberry_components_v2_patch.patch

[GoogleCodeExporter]

Apologies on my end, I've been very busy. 

1. lest we forget, this adds a dep to guice-multibindings-3.0.jar. Please help 
me remember to add this in the setup docs

2. What would you like to achieve by making {GuiceBerryEnvMain,TestWrapper} 
final?

The little syntax boilerplate saved by using inheritance for the sole purpose 
of overriding the constructor does not warrant, imho, the abuse of inheritance. 
I patched your CL, and modified the sequential to see what it looks like, and I 
like what I see, presented at the bottom of this message (from package 
declaration to the end, though the javadoc might need tweaks). BTW, before you 
have ideas to try to convince me that the class extending thingy is a good 
idea, you should chat with Marty, he'll tell you I'm a hopeless "class Foo 
extends Bar" hater :) And last I let myself be talked into it (that'd be 
GuiceBerryModule), I regretted it a bunch, as you know...

3. The new classes lack a copyright at the top

4. I don't like a GuiceBerryComponentsModule for multibindings (see "extends 
hater" bit above). At the least, it feels strictly better to change this (found 
in the test class):

      Multibinder<GuiceBerryEnvMain> envMainBinder = Multibinder.newSetBinder(binder(), GuiceBerryEnvMain.class);
      envMainBinder.addBinding().to(IncrementingGuiceBerryEnvMain.class);

to:

      Multibinder<GuiceBerryEnvMain> envMainBinder = GuiceBerryEnvMainMultibinder.multibinder(binder());
      envMainBinder.addBinding().to(IncrementingGuiceBerryEnvMain.class);

and it feels strictly better to go further and do:

      GuiceBerryEnvMainMultibinder.addBinding(binder()).to(IncrementingGuiceBerryEnvMain.class);

where "addBinding" here simply returns a 
LinkedBindingBuilder<GuiceBerryEnvMain>, i.e. the impl would look like this:

    public static LinkedBindingBuilder<GuiceBerryEnvMain> addBinding(Binder binder) {
      Multibinder<GuiceBerryEnvMain> envMainBinder = Multibinder.newSetBinder(binder, GuiceBerryEnvMain.class);
      return envMainBinder.addBinding();
    }

No duplication of code, nothing. Just a simpler, documentable, findable way to 
multibind GBEMains... I tried it and the tests still pass. What am I missing?

Thanks for the good work,

Z

**********************

package com.google.guiceberry.components;

import com.google.guiceberry.GuiceBerryEnvMain;

import javax.inject.Inject;
import java.util.Set;
import java.util.concurrent.Executors;

/**
 * A Guiceberry {@link com.google.guiceberry.GuiceBerryEnvMain} that is used internally to support the
 * execution of Multi-bound GuiceBerryEnvMains.
 *
 * <p>This class executes each bound env main sequentially in the order they were bound.
 *
 * @author jongerrish@google.com (Jonathan Gerrish)
 */
public class SequentialGuiceBerryEnvMainExecutor implements GuiceBerryEnvMain {

  private final GuiceBerryEnvMainExecutor delegate;

  @Inject
  public SequentialGuiceBerryEnvMainExecutor(Set<GuiceBerryEnvMain> envMains) {
    delegate = new GuiceBerryEnvMainExecutor(envMains, Executors.newSingleThreadExecutor());
  }

  public void run() {
    delegate.run();
  }
}

Original comment by zorze...@gmail.com on 18 Apr 2012 at 12:04

[abunyea]

Is anyone still working on this? I'd be willing to pick this up where it left off if there's still interest.

[zorzella]

Closing old/obsolete issues.