Style Guide Proposal: Test style, @Mock/@Captor and static imports

[Sky Malice]

Hello ja...@chromium.org

I would like to formally update the styleguide/java/java.md to call out some test style patterns that I find fairly common across the code base. I want our test code to be more homogenous, and I think documenting consensus will help us achieve this.

1. @Mock/@Captor should be preferred over inline creation of these objects.

2. Functions from commonly used test libraries, especially org.junit.Assert, org.mockito.Mockito, and org.mockito.ArgumentMatchers should be statically imported. These functions are very common in test code, and static imports help keep lines short, terse, and readable. There's often no ambiguity about where these functions are coming from, and since they are commonly statically imported in test files, this practice isn't surprising.

3. New test files should be in the same directory as the files under test.

Especailly, I find AI repeatedly creating inline Mock objects, and I am asking in code reviews many times per week to move them to member fields with @Mock annotations.

Proposed update https://crrev.com/c/7855165

Thanks,

Sky

[Nate Fischer]

Late followup from me, but I thought I'd give my 2c anyway

> @Mock/@Captor should be preferred over inline creation of these objects.

I'm OK with this. I think you've justified this nicely and it seems like a good styleguide addition.

I think I initially had the wrong interpretation of this, but after looking at the text in the CL I think this is fine. I initially thought this mean "you must use '@Mock' annotation, do not create 'Fake' subclasses or other ways of mocking out dependencies." I think the intended interpretation is "don't call Mockito.mock(Foo.class) because that's clunky," and I'm fine with that. Basically, I just want to allow test authors the flexibility to use patterns like "FakeSubClass extends RealClass" when that pattern makes more sense for the specific test case (I've had rare cases where "@Mock" results in some functional-but-unreadable test code).

> Functions from commonly used test libraries, especially org.junit.Assert, org.mockito.Mockito, and org.mockito.ArgumentMatchers should be statically imported. These functions are very common in test code, and static imports help keep lines short, terse, and readable. There's often no ambiguity about where these functions are coming from, and since they are commonly statically imported in test files, this practice isn't surprising.

IMO this is too strict to enforce across the entire codebase. I'm not opposed to individual test authors using these static imports when they want to, but I don't feel this needs to be strictly enforced upon everyone contributing to chromium. Here are some counterpoints:

• I think the readability impacts are minimal. IMO Assert.assertEquals" vs. "assertEquals" are both sufficiently readable and maintainable.
• We have a large existing volume of "Assert.assertEquals" in the codebase and I don't think it makes sense to ban the coding style used by so many other tests. I don't think it's worth the churn to update all the old tests to use the new preferred style.
• There's no functional benefit to one way or another, because they are exactly the same underlying code. This is purely a cosmetic change.

> New test files should be in the same directory as the files under test.

This isn't compatible with how Android WebView tests are structured and I'd really prefer not to change that. I understand why you want this for Chrome tests, but I don't think the styleguide is the right layer to enforce this.

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 visit https://groups.google.com/a/chromium.org/d/msgid/java/CAGLE5Td8CWhE%2BcLxTVS-35UaZLVXPZxWrW%2B-o_F2hV6eS_MTXg%40mail.gmail.com.