Style Guide Proposal: Adding go/clank-test-strategy

[Andrew Grieve]

Proposed CL (will send for review after 2 days of no comments).

Pasted below:

Googlers, see go/clank-test-strategy.

In summary:

• Use real dependencies when feasible and fast. Use mocks or fakes when necessary. Use Mockito’s @Mock most of the time, but write fakes for frequently used dependencies.

• Do not use Robolectric Shadows for Chromium code. Instead, use setForTesting() methods so that it is clear that test facilities exist.

• Use Robolectric when possible (when tests do not require native or rendering). Other times, use on-device tests with the @Batch annotation.

• Write integration tests when bugs are more likely to be in how/when code is called or due to interactions with other code. In other words, use integration tests when code is:

  • “application-like” code,
  • mainly defined by its behavior,
  • intended to be used in a small number of contexts (often only one),
  • internally simple.

[Nate Fischer]

> Other times, use on-device tests with the @Batch annotation.

Can we take this part out? Test batching has been a major source of maintenance pain for WebView tests.

> Write integration tests when bugs are more likely to be in how/when code is called or due to interactions with other code. In other words, use integration tests when code is:

I think this is redundant with the previous bullet point. Both boil down to picking unit testing or integration testing, depending on which strategy makes sense. I think we can defer to CL authors and CL reviewers to advise on the best strategy. Also, I suspect the 4 sub-bullets may not work very well for WebView (WebView is a library, so we seldom write "application-like" code). Overall, I'm concerned this is just going to confuse contributors and lead them to choose the wrong strategy.

Nate Fischer | Software Engineer | ntf...@google.com

--
You received this message because you are subscribed to the Google Groups "java" group.
To unsubscribe from this group and stop receiving emails from it, send an email to java+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CABiQX1Wo%3DENmC6Q1TXKNeZNYAgr9R-dri%2B-La0kvM9-ToqDLFQ%40mail.gmail.com.

[Andrew Grieve]

Thanks all for the feedback! PTAL at the changes. Specifically:

1) Added a mention of ResettersForTesting

2) Removed the bullet that Nate pointed out was redundant (I agree)

3) Elaborated on @Batch that PER_CLASS should be used for non-unit tests (Nate - does this address your concern, or has PER_CLASS batching caused pain as well?)

[Nate Fischer]

Thanks!

> Elaborated on @Batch that PER_CLASS should be used for non-unit tests (Nate - does this address your concern, or has PER_CLASS batching caused pain as well?)

Unfortunately we've had trouble with PER_CLASS batching as well. There have been several different cases where we had to un-batch the tests after exhausting all other ideas for deflaking.

Could we keep @Batch as a suggestion to make tests more efficient, but with the flexibility to not use the annotation if it compromises test flakiness?

Nate Fischer | Software Engineer | ntf...@google.com

[Andrew Grieve]

That's too bad. I've updated the text to mention @DoNotBatch, which hopefully makes it clear.

[Nate Fischer]

Thanks, the new update SGTM!

Nate Fischer | Software Engineer | ntf...@google.com