It would be useful to have a TestRule version of GuiceBerryRule

[GoogleCodeExporter]

MethodRule is semi-deprecated (it was deprecated, but I think more recently has 
been un-deprecated) and it doesn't work with certain new JUnit4 features such 
as RuleChain. Unfortunately, we can't make a non-breaking change to update 
GuiceBerryRule to TestRule as the target object is not passed into the apply 
method for TestRule. Attached is a proposed implementation of 
GuiceBerryTestRule (and if added, we should also probably deprecate 
GuiceBerryRule).

Original issue reported on code.google.com by fishe...@google.com on 29 Mar 2013 at 4:52

Attachments:

• GuiceBerryTestRule.java

[GoogleCodeExporter]

Thanks for the patch. Sorry I'm not familiar with TestRules, so as I asked 
before:

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

If you file that as a bug report/patch, make sure to explain what I'm getting 
by having a parallel "GuiceBerryTestRule" way of doing things, and if the other 
should be deprecated and why.

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

Also let me know what the test class would look like when it wants to use this. 
And, to reinforce (for my own benefit) what is the advantage of using the 
TestRule over what we have right now?

Thanks again,

Z

Original comment by zorze...@gmail.com on 8 Apr 2013 at 10:59

[GoogleCodeExporter]

To answer your questions

1) Why you need a parallel "GuiceBerryTestRule"

MethodRule does not and cannot support RuleChain.

MethodRule also doesn't support the @ClassRule annotation, but I don't think 
the new TestRule-based GuiceBerry rule should work with the @ClassRule 
annotation

For more details, see 
http://java.dzone.com/articles/junit-49-class-and-suite-level-rules and 
https://github.com/junit-team/junit/blob/master/doc/ReleaseNotes4.9.md

2) Should the old way of doing things be deprecated

If you think the latest version of GuiceBerry should work with JUnit 4.8 and 
earlier, then you should not deprecate the current GuiceBerry rule. Note that 
MethodRule was deprecated in 4.9, but was undeprecated in a later release

3) What the test class would look like when it wants to use this

The syntax for using a TestRule-based rule in JUnit4 is the same as the syntax 
of using a MethodRule-based rule. Note that if your rule needs an instance of 
the test class, then with TestRule you will have to pass "this" to your rule's 
constructor or creational method. That's why the creational methods in the 
patch have "target" parameters.

Note that the supplied patch should be modified to cause evalulate() to throw 
an exception if description.isSuite() returns true (because GuiceBerry cannot 
be used as a class rule)

Original comment by kcoo...@google.com on 9 Apr 2013 at 12:01

[GoogleCodeExporter]

Let me see if I get this straight:

A) the test would change from:

  @Rule
  public final GuiceBerryRule guiceBerry = new GuiceBerryRule(FooEnv.class);

to:

  @Rule
  public final GuiceBerryTestRule guiceBerry = new GuiceBerryTestRule(this, FooEnv.class);

or (if one uses the "create" static method):

  @Rule
  public final GuiceBerryTestRule guiceBerry = GuiceBerryTestRule.create(this, FooEnv.class);

what is the advantage of having this "create" method again? It seems just more 
typing. Is it just idiomatic or something? So, the downsides seem to me (from 
most to least serious):

A- 1) the error-prone prospect possibility that someone passes something other 
than "this" to GBTR's constructor -- say "new GBTR("pink", FooEnv.class)"

A- 2) aside from the longer name, it's 6 more characters to type ("this, ")

A- 3) for better or worse, I also can't have my @Rule be "static" (not that I 
see a reason for it, anyway, just saying)

The upside:

A+ 1) it can be used in a RuleChain

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

So if I got this straight, there's never an advantage/reason to using BGTR in a 
test per se -- it becomes necessary only when the test uses a RuleChain 
(*instead* of GBR or GBTR).

If this understanding is correct, I'm leaning towards:

1. having this new class be called GuiceBerryRuleForChain or some such thing

2. keep GBR as is, being the by-default-documented way of using GB in JU4.

3. add a note to GBR's javadoc stating that, if one wishes to use GB in a 
RuleChain, see GBRFC or some such thing.

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

If you don't agree this is a good course of action, let me know why, otherwise 
please send a CL with:

a. the changes above (including GBR's javadoc changes)

b. the "throw on isSuite" change you suggested

c. tests for this new class

Thanks,

Z

Original comment by zorze...@gmail.com on 9 Apr 2013 at 4:59

[GoogleCodeExporter]

A) Correct

A- 1) This is common enough in TestRules that it is unlikely to be a problem in 
practice.

A- 2) Ack

A- 3) Well, we could make it static if it is a ClassRule, but it is unclear 
what that would mean in the context of GB (and if we throw on isSuite, then we 
are back to not being static). But this can't fairly be considered a downside 
as it is already the case.

A+ 1) Pretty much, but this is important, especially for a Rule such as GBR. It 
is the only way to guarantee that rules execute in a certain order, and in most 
cases in my experience it matters very much where in the series of rules GBR 
executes as other rules may depend on the injected fields, or the started 
servers, or etc. that are setup by GBR.

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

I do not like the idea of having GBTR as a second-class citizen as you propose 
here. I believe that the downsides you listed above are all very small, and the 
order of GBR relative to other rules is frequently very important. Therefore, I 
stick by my original proposal, make GBTR the preferred mechanism for using GB 
in JUnit4, and deprecate GBR. If that is a non-starter than I prefer the 
mechanism I have used elsewhere, use an adapter rule that converts a MethodRule 
to a TestRule, over your proposed solution (such a rule could potentially 
become a standard part of JUnit4).

Marc

Original comment by fishe...@google.com on 9 Apr 2013 at 5:31

[GoogleCodeExporter]

It's possible that in the future, JUnit will enable other features (besides 
RuleChain) for TestRule but not MethodRule. So GuiceBerryRuleForChain is 
probably not the best name.

I have no strong feelings about constructors vs. static creational methods.

Original comment by kcoo...@google.com on 9 Apr 2013 at 5:52

[GoogleCodeExporter]

This has been a new issue for over a year. Is there any movement on getting 
this added?

Original comment by karl.kos...@lab49.com on 8 Oct 2014 at 5:37

[zorzella]

FYI, I'm still waiting for someone to finish to polish the patch that was sent, and I'll happily apply it. Here are the invariants:

1. I'm keeping GuiceBerryRule around. Even if I wanted to kill it, I couldn't, and, unless you need a TestRule, using GBR is a safer and more compact.
2. I'm happy to name the new rule GuiceBerryTestRule

Essentially I just need a patch with the "throws" issue resolved, javadoc and tests.

[kcooney]

@zorzella where can I find the patch you are referring to? What is the "throws issue"?

[zorzella]

Hey, Kevin.

the "patch" is the attachment in the first comment (i.e. the bug report itself) -- this predates the conversion to GitHub, so it's not like it's a branch or something.

The "throws issue" is what I think you yourself described: "Note that the supplied patch should be modified to cause evalulate() to throw an exception if description.isSuite() returns true (because GuiceBerry cannot be used as a class rule)"

I strongly encourage you to re-read my post that starts with "Let me see if I get this straight:", as my proposal for the "polish" I talk about is described there.

Peace,

Z

[zorzella]

Closing old/obsolete issues.