GuiceBerryModule should be installable multiple times

[GoogleCodeExporter]

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

The problem is that each separate testing infrastructure module may install(new 
GuiceBerryModule()), but GuiceberryModule can only be installed once per 
injector and multiple bindings errors are reported.

It is possible to refactor this and install the GuiceBerryModule at the top 
level, but this results in one extra top level Guice module per component to 
serve as the Env for that components tests. For example, If Project A uses 
Project B and Project C's testing infrastructure we would need:-

ProjectATopLevelEnv - installs(GuiceBerryModule(), ProjectATestingModule(), 
ProjectBTestingModule(), ProjectCTestingModule())
ProjectBTopLevelEnv - installs(GuiceBerryModule(), ProjectBTestingModule());
ProjectCTopLevelEnv - installs(GuiceBerryModule(), ProjectCTestingModule();

Possible solution 1:-

Since Guice de-dupes modules based on object equality, if GuiceBerryModule was 
made final and implemented equals/hashCode, each of the projects above could 
install GuiceBerryModule() without any conflict, the consequence of this would 
be the projects could no longer subclass GuiceberryModule and would break 
existing projects. (Should we encourage/enforce subclassing or composition of 
GuiceberryModule?

Possible solution 2:-

We could simply create a new ShareableGuiceberryModule extends GuiceberryModule 
which would not break existing users, however each piece of testing 
infrastructure that users wish to share would need to be refactored. It also 
creates another class in the public API and may result in confusion?

public final class ShareableGuiceBerryModule extends GuiceBerryModule {

  @Override
  public boolean equals(Object o) {
    return o != null && this.getClass().equals(o.getClass());
  }

  @Override
  public int hashCode() {
    return this.getClass().hashCode();
  }
}

Possible solution 3:-

Extract all contents of GuiceBerryModule to a new package private class, 
SharableGuiceberryModule, which is made final and implements equals and hash 
code, GuiceBerryModule is just a shell which installs SharableGuiceBerryModule.

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

[GoogleCodeExporter]

You ask: Should we encourage/enforce subclassing or composition of 
GuiceBerryModule?

<sigh>Ideally I could go back in time and declare GBM to be final. There is no 
compelling reason to extend it, instead of extending AbstractModule and 
installing GBM (there _used_ to be a good reason in the old GuiceBerry 2.0 
days, which does not exist anymore).

Therefore, my preferred solution would be #1, but that requires fixing the 
Google codebase.

Now, I would be delighted if you were to decide to be a good Samaritan and fix 
the 119 usages of "extends GBM" in the Google codebase (search -l for 
com.google.guiceberry.GuiceBerryModule and | xargs grep "extends 
GuiceBerryModule") so we could just make it "final". But I'm not going to ask 
you to do it, either, even if it is a safe and mechanical change.

I am deciding which, between #2 and #3, is less bad and I'll get back to you 
(unless you tell me you're going for #1).

Original comment by zorze...@gmail.com on 8 Feb 2012 at 10:25

[GoogleCodeExporter]

I went with option 3, I think this is better than option 2 since it should make 
all existing uses of GuiceBerryModule be re-installable... see attached patch.

However, I also prefer option 1, and I am happy to make that change to the 
Google codebase if you are supportive. It will break external users though. Let 
me know which you prefer.

Original comment by jongerr...@google.com on 8 Feb 2012 at 10:32

Attachments:

• guiceberry_issue21_v1.patch

[GoogleCodeExporter]

Although, going with option 1 would mean there wouldn't be a natural place to 
add hooks for

bindTestWrapper()...
bindGuiceBerryEnvMain()...

This really depends on your feelings of wanting to hide the implementation 
using multibinder to support multiple bindings, some people do, especially if 
building extension points to be used by others as the whole multibinder syntax 
isn't really a clean API, and harder to locate code making those multi bindings 
than if there was a cleaner more specific API.

As an alternative to creating these hooks in a super class (e.g: 
GuiceBerryModule), it could be done with a static helper, e.g:

  public static LinkedBindingBuilder<TestWrapper> bindTestWrapper(Binder binder) {
    return Multibinder.newSetBinder(binder, TestWrapper.class, Internal.class).addBinding();
  }

What are your thoughts?

Original comment by jongerr...@google.com on 8 Feb 2012 at 10:43

[GoogleCodeExporter]

1. Doing #3 would, indeed, make things strictly better, so let's start with 
that.

2. I feel dirty telling people to extend GBE, but we'll talk about the 
multibinding syntax issue in issues #22 and #23.

3. In fact, it all comes back to me now -- i.e. why GBE is not final as it 
should. Well, it turns out that I need it not final to support the old 
GuiceBerryJunit3 (i.e. pre-3.0 GuiceBerry). Which is rather unfortunate, since 
there are lots of uses of this in google codebase.

4. Back to your patch, there is a problem with the equals method, and with the 
TestScope thing -- two GBEs will be considered the same but won't share the 
same TestScope, which is a problem. I think this is a solvable issue, though. 
Wanna take a stab?

Original comment by zorze...@gmail.com on 12 Feb 2012 at 5:04

[GoogleCodeExporter]

You're right, I missed that the testscope is created in a non-re-installable 
module, so will be different across instances.... if I understand correctly, I 
think the best thing to do here is to move both protected members universe + 
testscope inside of the SharableGuiceBerryModule and fix any subclasses that 
are using it.

Case 1: Binding within test scope:-

bind(Foo.class).in(super.testScope);

would become

bind(Foo.class).in(TestScoped.class);

case 2: Binding another scope annotation to test scope:-

bindScope(RequestScoped.class, super.testScope);

...but I'm not sure exactly how to fix this.

Original comment by jongerr...@google.com on 13 Feb 2012 at 5:22

[GoogleCodeExporter]

I _think_ if you move the instantiation of the test scope inside 
GuiceBerryUniverse you can make things happy. I can't spend much time on this 
right now, so I'll ask you to puzzle this out a bit to see if you can make it 
work. If you can't, I'll take a proper look.

Original comment by zorze...@gmail.com on 13 Feb 2012 at 6:06

[GoogleCodeExporter]

I took a good look at the classes. I can fix the difficulties with TestScope. 
It will be kind of ugly (internally), but it will work.

While I was looking at this, though, I come to think that it's needless to 
introduce a SharableGuiceBerryModule. What I'm thinking of doing is saying: if 
you want to get the Guice feature of sharing GBMs, just make sure not to extend 
it. I think it's a reasonable demand, and will provide a bit of a carrot for 
people to stop extending GBM. I'll also change my internal adapter class so it 
does not extend GBM, which will allow me (later) to make it final (i.e. if and 
when the google codebase stops extending it).

I should also add @Deprecated to the pre-3.0 classes, to give more of stick for 
people to stop using it...

I'll post a patch here by next week (I'm out for the next couple of days).

Z

Original comment by zorze...@gmail.com on 15 Feb 2012 at 9:59

[GoogleCodeExporter]

I agree with you, its just simpler to say, if you want to install multiple 
GuiceBerryModules, just make sure you avoid extending GuiceBerryModule, and 
then we can try and deprecate the extending of GuiceBerryModule across the 
codebase and eventually make GuiceBerryModule final. I'm willing to help with 
this, I think it will be more grunt work than technical difficulties....

GuiceBerryModule also has two protected members, universe and testScope. It 
looks like universe is not referenced in the codebase, but testScope has two 
types of references:-

Case 1: Binding within test scope:-

bind(Foo.class).in(super.testScope);

would become

bind(Foo.class).in(TestScoped.class);

which is an easy fix.

case 2: Binding another scope annotation to test scope:-

bindScope(RequestScoped.class, super.testScope);

Which is less easy... I don't think its possible in Guice to do something like 
bind(MyScope.class).to(TestScoped.class); and therefore indirectly bind 
MyScope.class annotated things to the same scope as TestScoped. If you could 
make GuiceBerryModule.testScope be a singleton instance e.g: 
TestScope.INSTANCE, similar to ServletScopes.REQUEST_SCOPE then you could fix 
this case as:-

bindScope(RequestScoped.class, TestScope.INSTANCE);

I tried to make this change to GuiceBerry, but it was hard for me given the 
DeprecatedGuiceBerryModule and the tests that use it by passing in a 
GuiceBerryUniverse (which is then used to create the test scope). I think you 
wanted GuiceBerryUniverse to be a singleton, but its not for the purpose of 
testing?

....I suppose it depends if you want to expose this in GuiceBerry... I guess 
some tests which deal with request / session scopes just want to do the easy 
thing and tie them to TestScoped during tests... is that a legitimate use case 
do you think? We could probably just re-work their tests to use some other kind 
of simple scope which spans the duration of a test.

What do you think? Anyway, let me know what I can do to help.

Cheers, Jonathan.

Original comment by jongerr...@google.com on 21 Feb 2012 at 11:24

[GoogleCodeExporter]

> case 2: Binding another scope annotation to test scope:-

> bindScope(RequestScoped.class, super.testScope);

> Which is less easy... I don't think its possible in Guice to do something 
like 
> bind(MyScope.class).to(TestScoped.class); and therefore indirectly bind 
MyScope.class annotated things to 
> the same scope as TestScoped

Will you please confirm that there's no better syntax for this? The most 
obvious way to work around this problem is to create in GBM this method

public void bindToTestScope(Class<? extends Scope> toBeBound) {
  bindScope(toBeBound, testScope);
}

Which I'll gladly add (if, again, there's no better syntax to provide for this).

Original comment by zorze...@gmail.com on 22 Feb 2012 at 5:36

[GoogleCodeExporter]

I think the best syntax for this would be to make TestScope have a singleton 
instance, then existing users of GuiceBerryModule.testScope in case 2 could 
migrate to

bindScope(MyScope.class, TestScope.INSTANCE);

I was trying to refactor GuiceBerryModule to support this but the tests that 
use DeprecatedGuiceBerryModule were tying me in knots :-)

Also, the problem of adding a bindToTestScope method is that it doesn't allow 
us to make GuiceBerryModule final, but maybe you think this is something that 
can be done in steps?

I guess to support the sharing of modules we can still add the equals + 
hashcode to GuiceBerryModule?

Original comment by jongerr...@google.com on 22 Feb 2012 at 5:04

[GoogleCodeExporter]

I think the only solution here is to make TestScope a Singleton, but I want to 
bounce this first.

Original comment by zorze...@google.com on 24 Feb 2012 at 1:44

[GoogleCodeExporter]

FYI, In Guice 3 exact duplicate bindings are ignored (instead of an exception 
being thrown). This might make fixing this easier.
  http://code.google.com/p/google-guice/wiki/Guice30

Original comment by jessewil...@google.com on 24 Feb 2012 at 6:30

[GoogleCodeExporter]

Jesse,

this does not seem to help at all when there are scopes bound in a module, 
unless the scope is a singleton (static), which I understand you said in our IM 
that you didn't really like. Besides, in the case of TestScope, it really can't 
be a static, since TestScope is _not_ stateless, and multiple injectors would 
potentially (even if rarely) step on each other's toes.

I filed a feature request to allow for scope aliasing (which would make things 
better):

http://code.google.com/p/google-guice/issues/detail?id=687

But, for now (until the bug is fixed), I'll just work around the problem. I 
expect to post a patch by the end of the day.

Original comment by zorze...@gmail.com on 27 Feb 2012 at 7:28

[GoogleCodeExporter]

Jonathan,

please look at the attached patch.

Once it's submitted, and a Guiceberry release is cut/pushed to Google codebase, 
you'd need to fix the testScope usages in the Google codebase, then we'd remove 
testScope from GuiceBerryModule, and then, with a simple equals and hashCode, 
we're done here.

Original comment by zorze...@gmail.com on 27 Feb 2012 at 7:45

Attachments:

• TEST_SCOPE_PATCH.patch

[GoogleCodeExporter]

Nice job, I think that will work!

So there are just two usages of test scoped in the code base, case 1 is easy to 
fix, but what do you suggest for case 2: Do you think its reasonable for users 
to bind their own scope annotations to gbm.buildTestScope(), or is it better to 
prevent this and say, if you want to scope your own annotations to the scope of 
a running test, you'd need to sort out your own scope. I'll investigate.... 
thanks for the patch!

Original comment by jongerr...@google.com on 27 Feb 2012 at 8:52

[GoogleCodeExporter]

My idea is for case #2 to be changed to gbm.buildTestScope(). There's a slight 
change of behavior, as they'd be getting a new instance of TestScope, but I 
can't think of any way in which this would actually change any perceivable 
behavior.

I'll submit. I'll even cut the release, but I depend on you to update the 
google codebase (including bring the new GuiceBerry jar in).

BTW, did you see my reply to issue #22?

Original comment by zorze...@gmail.com on 27 Feb 2012 at 9:45

[GoogleCodeExporter]

OK, sounds good. Is it possible to make GuiceBerryModule.{universe,testScope} 
package private, to stop sub classes accessing it? Also, can I submit a patch 
for the equals and hashcode before you cut the release. I've already made one 
change to the google code base, and it looks like there is just one more to go, 
but I'll need the release to be cut first with this change in it.

Oh, yes, I say the reply to issue #22, I'll address that...

Original comment by jongerr...@google.com on 27 Feb 2012 at 9:51

[GoogleCodeExporter]

I will make universe package private at the same time I get rid of testScope. 
Wait for equals/hashCode until testScope is done with.

Original comment by zorze...@gmail.com on 27 Feb 2012 at 9:59

[GoogleCodeExporter]

Ok, I've made GuiceBerry 3.2.0 release. I need you to:

1. prepare a release notes statement
2. bring it to google code

Once this is done, I'll make the further changes.

Original comment by zorze...@gmail.com on 2 Mar 2012 at 9:32

[GoogleCodeExporter]

Ok, here's the patch that will do the trick. Take a look to see if it's good. 
Please send me a patch with tests for this, and I'll merge them and submit.

Original comment by zorze...@gmail.com on 6 Mar 2012 at 11:21

Attachments:

• GB_EQUALS.txt

[GoogleCodeExporter]

Here is your original patch GB_EQUALS.txt along with a test to assert 
re-installation works merged together. You can just apply this and submit if 
you're happy with it. I couldn't think of a better way of testing other than 
trying to install GuiceBerryTwice and not having an exception.

Original comment by jongerr...@google.com on 7 Mar 2012 at 6:46

Attachments:

• GB_EQUALS_WITH_TEST.patch

[GoogleCodeExporter]

Here is a small patch to make GuiceBerryModule final. If we could include both 
GB_EQUALS_WITH_TEST.patch and this patch in a new release then I can push this 
to the google code base. Currently a new "extends GuiceBerryModule" pops up 
every other day.. its like playing whac-a-mole ;-)

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

Attachments:

• Make_GuiceBerryModule_final.patch

[GoogleCodeExporter]

GuiceBerry 3.3.1 available for download: GBM implements equals and hashCode and 
is shareable. This fixes this bug. As I told you offline, making it final 
should be a separate release -- it will be part of the next release, when #22 
and #23 are also fixed.

Original comment by zorze...@gmail.com on 24 Mar 2012 at 8:16

• Changed state: Fixed