Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2770393002:
Add setup action interface (Closed)
|
Created:
3 years, 9 months ago by the real yoland Modified:
3 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd setup action interface
Interface to ensure TestRule that implements it specifies if the rule
would run a set-up process before @Before method.
BUG=640116
Review-Url: https://codereview.chromium.org/2770393002
Cr-Commit-Position: refs/heads/master@{#465062}
Committed: https://chromium.googlesource.com/chromium/src/+/c236847d7da49e1799cc64d9decb77e92a3995f4
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #
Total comments: 5
Patch Set 3 : address comments #Patch Set 4 : Move SetUpStatement to its own file #
Total comments: 4
Patch Set 5 : address comments #
Total comments: 4
Patch Set 6 : Add tests #
Dependent Patchsets: Messages
Total messages: 46 (15 generated)
yolandyan@chromium.org changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org
JavaBridgeActivityTestRule is just an example that implements this SetUpTestRule interface. It will be moved to a different CL after this one lands.
Not a huge fan that the interface does not require using a SetupStatement but don't see a way to do that. non-owner lgtm https://codereview.chromium.org/2770393002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java (right): https://codereview.chromium.org/2770393002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java:18: * Set whether the TestRule should run setUp automatically and return the TestRule. This could probably use clearer wording. Maybe... /* Sets whether the TestRule should run setUp automatically. @returns itself for convenience */ https://codereview.chromium.org/2770393002/diff/1/content/public/android/java... File content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java (right): https://codereview.chromium.org/2770393002/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java:63: return super.apply(new SetUpStatement(base, this, mRunSetUp), desc); guessing this needs to be SetUpTestRule.SetUpStatement
https://codereview.chromium.org/2770393002/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java (right): https://codereview.chromium.org/2770393002/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java:18: * Set whether the TestRule should run setUp automatically and return the TestRule. On 2017/03/27 at 18:40:59, mikecase wrote: > This could probably use clearer wording. Maybe... > > /* > Sets whether the TestRule should run setUp automatically. > > @returns itself for convenience > */ Done https://codereview.chromium.org/2770393002/diff/1/content/public/android/java... File content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java (right): https://codereview.chromium.org/2770393002/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java:63: return super.apply(new SetUpStatement(base, this, mRunSetUp), desc); On 2017/03/27 at 18:40:59, mikecase wrote: > guessing this needs to be SetUpTestRule.SetUpStatement Done
I'm wondering if there's a way we can reduce the boilerplate here. Additionally, like case, I'm concerned about the inability to ensure the use of SetUpStatement. https://codereview.chromium.org/2770393002/diff/20001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java (right): https://codereview.chromium.org/2770393002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java:35: * Custom Statment for SetUpTestRules. nit: Statment -> Statement https://codereview.chromium.org/2770393002/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java (right): https://codereview.chromium.org/2770393002/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java:67: public JavaBridgeActivityTestRule shouldSetUp(boolean shouldSetUp) { So every rule supporting this behavior would need to do something like this, w/ its own boolean & shouldSetUp implementation?
In terms of naming, maybe we can call this maybe call it SetUpRule, and create
an abstract class called SetUpTestRule. Or maybe call it AutoSetUp, and an
abstract class called AutoSetUpTestRule
```
public interface SetUpRule {}
public abstract class SetUpTestRule implement SetUpRule, TestRule {}
```
or
```
public interface AutoSetUp {}
public abstract class AutoSetUpTestRule implement AutoSetUp, TestRule{}
```
https://codereview.chromium.org/2770393002/diff/20001/base/test/android/javat...
File base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java
(right):
https://codereview.chromium.org/2770393002/diff/20001/base/test/android/javat...
base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java:35: *
Custom Statment for SetUpTestRules.
On 2017/03/28 at 01:13:28, jbudorick wrote:
> nit: Statment -> Statement
Done
https://codereview.chromium.org/2770393002/diff/20001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java
(right):
https://codereview.chromium.org/2770393002/diff/20001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java:67:
public JavaBridgeActivityTestRule shouldSetUp(boolean shouldSetUp) {
On 2017/03/28 at 01:13:28, jbudorick wrote:
> So every rule supporting this behavior would need to do something like this,
w/ its own boolean & shouldSetUp implementation?
ya, one way to decrease the boilerplate code is to create an abstract class that
implements these methods.
https://codereview.chromium.org/2770393002/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java (right): https://codereview.chromium.org/2770393002/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java:67: public JavaBridgeActivityTestRule shouldSetUp(boolean shouldSetUp) { On 2017/03/28 01:32:50, the real yoland wrote: > On 2017/03/28 at 01:13:28, jbudorick wrote: > > So every rule supporting this behavior would need to do something like this, > w/ its own boolean & shouldSetUp implementation? > > ya, one way to decrease the boilerplate code is to create an abstract class that > implements these methods. Let's talk about this offline.
friendly ping
On 2017/03/28 01:36:59, jbudorick wrote: > https://codereview.chromium.org/2770393002/diff/20001/content/public/android/... > File > content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java > (right): > > https://codereview.chromium.org/2770393002/diff/20001/content/public/android/... > content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java:67: > public JavaBridgeActivityTestRule shouldSetUp(boolean shouldSetUp) { > On 2017/03/28 01:32:50, the real yoland wrote: > > On 2017/03/28 at 01:13:28, jbudorick wrote: > > > So every rule supporting this behavior would need to do something like this, > > w/ its own boolean & shouldSetUp implementation? > > > > ya, one way to decrease the boilerplate code is to create an abstract class > that > > implements these methods. > > Let's talk about this offline. ^^ this afternoon?
On 2017/04/03 at 19:13:32, jbudorick wrote: > On 2017/03/28 01:36:59, jbudorick wrote: > > https://codereview.chromium.org/2770393002/diff/20001/content/public/android/... > > File > > content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java > > (right): > > > > https://codereview.chromium.org/2770393002/diff/20001/content/public/android/... > > content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeActivityTestRule.java:67: > > public JavaBridgeActivityTestRule shouldSetUp(boolean shouldSetUp) { > > On 2017/03/28 01:32:50, the real yoland wrote: > > > On 2017/03/28 at 01:13:28, jbudorick wrote: > > > > So every rule supporting this behavior would need to do something like this, > > > w/ its own boolean & shouldSetUp implementation? > > > > > > ya, one way to decrease the boilerplate code is to create an abstract class > > that > > > implements these methods. > > > > Let's talk about this offline. > > ^^ this afternoon? sure, ty in advance!
Changed
woah, thanks for the suggestion! This actually works much better
lgtm w/ q https://codereview.chromium.org/2770393002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/TestRuleUtils.java (right): https://codereview.chromium.org/2770393002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/TestRuleUtils.java:14: public class TestRuleUtils { What else would we conceivably put in here? Why shouldn't we have this be specific to setup?
On 2017/04/04 00:49:20, jbudorick wrote: > lgtm w/ q errr not lgtm (only to undo approval) please add tests for this to base_junit_tests :) > > https://codereview.chromium.org/2770393002/diff/80001/base/test/android/javat... > File base/test/android/javatests/src/org/chromium/base/test/TestRuleUtils.java > (right): > > https://codereview.chromium.org/2770393002/diff/80001/base/test/android/javat... > base/test/android/javatests/src/org/chromium/base/test/TestRuleUtils.java:14: > public class TestRuleUtils { > What else would we conceivably put in here? Why shouldn't we have this be > specific to setup?
On 2017/04/04 at 00:50:40, jbudorick wrote: > On 2017/04/04 00:49:20, jbudorick wrote: > > lgtm w/ q > > errr not lgtm (only to undo approval) > > please add tests for this to base_junit_tests :) > > > > > https://codereview.chromium.org/2770393002/diff/80001/base/test/android/javat... > > File base/test/android/javatests/src/org/chromium/base/test/TestRuleUtils.java > > (right): > > > > https://codereview.chromium.org/2770393002/diff/80001/base/test/android/javat... > > base/test/android/javatests/src/org/chromium/base/test/TestRuleUtils.java:14: > > public class TestRuleUtils { > > What else would we conceivably put in here? Why shouldn't we have this be > > specific to setup? Done
https://codereview.chromium.org/2770393002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/TestRuleUtils.java (right): https://codereview.chromium.org/2770393002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/TestRuleUtils.java:14: public class TestRuleUtils { On 2017/04/04 at 00:49:19, jbudorick wrote: > What else would we conceivably put in here? Why shouldn't we have this be specific to setup? Not sure what would be added in the future, I think it's possible we will have more utility methods for TestRules in the future. Changed to TestRuleSetUp.java
Test looks fine. Just the naming left. https://codereview.chromium.org/2770393002/diff/100001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/util/TestRuleSetUp.java (right): https://codereview.chromium.org/2770393002/diff/100001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/util/TestRuleSetUp.java:24: * public TestRule mRule = TestRuleUtils.autoSetUp(new MyTestRule()); TestRuleSetUp.autoSetUp(...) seems redundant.
I would argue boilerplate code solution (patch 4) is preferable comparing to this https://codereview.chromium.org/2770393002/diff/120001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/util/DisableIfTest.java (right): https://codereview.chromium.org/2770393002/diff/120001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/util/DisableIfTest.java:17: import org.chromium.testing.local.LocalRobolectricTestRunner; ugh, please ignore this, macro command auto organized import order https://codereview.chromium.org/2770393002/diff/120001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/util/SetUpUtils.java (right): https://codereview.chromium.org/2770393002/diff/120001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/util/SetUpUtils.java:16: if (f.getType().equals(callback.getClass()) hacky, also has an assumption that there is only one X type TestRule This also has to look for fields for each method run.
Patchset #7 (id:120001) has been deleted
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2770393002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/SetUpStatement.java (right): https://codereview.chromium.org/2770393002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/SetUpStatement.java:13: * The statment calls {@link SetUpTestRule#setUp} before test run based on rephrase: Calls {@link SetUpTestRule.setUp()} before evaluating {@link ...#base} if {@link ...#shouldSetUp} is true. https://codereview.chromium.org/2770393002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java (right): https://codereview.chromium.org/2770393002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java:9: /** This comment should mention the relation to SetUpStatement, too.
https://codereview.chromium.org/2770393002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/SetUpStatement.java (right): https://codereview.chromium.org/2770393002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/SetUpStatement.java:13: * The statment calls {@link SetUpTestRule#setUp} before test run based on On 2017/04/10 at 19:08:45, jbudorick wrote: > rephrase: Calls {@link SetUpTestRule.setUp()} before evaluating {@link ...#base} if {@link ...#shouldSetUp} is true. Done https://codereview.chromium.org/2770393002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java (right): https://codereview.chromium.org/2770393002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java:9: /** On 2017/04/10 at 19:08:45, jbudorick wrote: > This comment should mention the relation to SetUpStatement, too. Done
I'd still like a few tests for this in base_junit_tests. https://codereview.chromium.org/2770393002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java (right): https://codereview.chromium.org/2770393002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java:10: * Interface to ensure TestRule that implements it specifies if it would run a set-up process An interface for TestRules that can be configured to automatically run set-up logic prior to @Before. https://codereview.chromium.org/2770393002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java:13: * TestRule that implements this interface should return a {@link SetUpStatement} with TestRules that implement this interface should return a {@link SetUpStatement} from their {@link TestRule#apply} method.
https://codereview.chromium.org/2770393002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java (right): https://codereview.chromium.org/2770393002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java:10: * Interface to ensure TestRule that implements it specifies if it would run a set-up process On 2017/04/12 at 13:04:03, jbudorick wrote: > An interface for TestRules that can be configured to automatically run set-up logic prior to @Before. Done https://codereview.chromium.org/2770393002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/SetUpTestRule.java:13: * TestRule that implements this interface should return a {@link SetUpStatement} with On 2017/04/12 at 13:04:03, jbudorick wrote: > TestRules that implement this interface should return a {@link SetUpStatement} from their {@link TestRule#apply} method. Done
The CQ bit was checked by yolandyan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org Link to the patchset: https://codereview.chromium.org/2770393002/#ps160001 (title: "Add tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
yolandyan@chromium.org changed reviewers: + nyquist@chromium.org
add +nyquist for base owner stamp
base/BUILD.gn lgtm
The CQ bit was checked by yolandyan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1492466340708740,
"parent_rev": "0559cf83295ce57b37d8dff1a5358d6dd11b3c64", "commit_rev":
"c236847d7da49e1799cc64d9decb77e92a3995f4"}
Message was sent while issue was closed.
Description was changed from ========== Add setup action interface Interface to ensure TestRule that implements it specifies if the rule would run a set-up process before @Before method. BUG=640116 ========== to ========== Add setup action interface Interface to ensure TestRule that implements it specifies if the rule would run a set-up process before @Before method. BUG=640116 Review-Url: https://codereview.chromium.org/2770393002 Cr-Commit-Position: refs/heads/master@{#465062} Committed: https://chromium.googlesource.com/chromium/src/+/c236847d7da49e1799cc64d9decb... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/chromium/src/+/c236847d7da49e1799cc64d9decb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
