Ignore fields that are annotated with the @MockBean and @SpyBean annotations of Spring test

[uhafner]

This is a small followup to #477. When you are testing Spring (Boot) applications then you are replacing controllers that are normally injected via @Autowired with stubs, mocks or spies from Mockito. This is done with two similar annotations:

• @MockBean
• @SpyBean

So it would be helpful if these are ignored as well.

[uhafner]

Hmm, it seems that the dependency org.springframework.boot:spring-boot-test:2.7.5 causes the problem. I tried locally with 2.4.0, but that does not work as well. Is there a way to specify an exclusion in those dependencies? I tried with { exclude group: 'org.springframework.boot', module: 'spring-boot' } but that does not work in the test map.

[lazaroclapp]

Hmm, it seems that the dependency org.springframework.boot:spring-boot-test:2.7.5 causes the problem. I tried locally with 2.4.0, but that does not work as well. Is there a way to specify an exclusion in those dependencies? I tried with { exclude group: 'org.springframework.boot', module: 'spring-boot' } but that does not work in the test map.

Haven't had a chance to look at this in any level of detail, but one thing that seems to be happening is our existing tests failing with java.lang.NoClassDefFoundError. It is possible that spring-boot-test:2.7.5 is pulling in a more recent version of some common test dependency (e.g. mockito?) which we are using, which is no longer compatible with how we use it in tests.

[uhafner]

Hmm, it seems that the dependency org.springframework.boot:spring-boot-test:2.7.5 causes the problem. I tried locally with 2.4.0, but that does not work as well. Is there a way to specify an exclusion in those dependencies? I tried with { exclude group: 'org.springframework.boot', module: 'spring-boot' } but that does not work in the test map.

Haven't had a chance to look at this in any level of detail, but one thing that seems to be happening is our existing tests failing with java.lang.NoClassDefFoundError. It is possible that spring-boot-test:2.7.5 is pulling in a more recent version of some common test dependency (e.g. mockito?) which we are using, which is no longer compatible with how we use it in tests.

spring-boot-test depends "only" on spring-boot and that causes the pain. This is the reason why I tried to use the exclude for spring boot which pulls in a lot of transitive dependencies. I'm almost sure that there is a conflict.

I'm a Maven user so I am not familiar with Gradle: in Maven you would simply exclude the transitive dependency, it should not be required during compile time for these two annotations. But the syntax in your scripts seems to not allow exclusions, at least I found no way to add one.

(The PR is not really important for me, I just thought it might make sense to add these annotations because I needed to add some suppressions in my code. So we should not spend too much time to fix it, if the testing framework does not support it out of the box.)

[msridhar]

Sorry for the long delay in looking at this. I don't think the transitive dependency is the issue. For the failing test I see org.springframework.boot.test.mock.mockito.SpringBootMockResolver.resolve at the top of the stack, and SpringBootMockResolver is inside spring-boot-test-2.7.5.jar. I think unfortunately there is some interaction between spring-boot-test and Mockito that makes our existing tests unhappy.

An alternative here is to just "stub out" the relevant @MockBean and @SpyBean annotations within NullAway itself, rather than taking the dependence. We already do this for a bunch of Android-related types. See here:

https://github.com/uber/NullAway/tree/1ff88b64e527cdb529f5df38ab5ade8dc01b0352/nullaway/src/test/resources/com/uber/nullaway/testdata/androidstubs

Could we take the same approach in this PR, so we don't even need the dep on spring-boot-test?

[uhafner]

Thanks for the pointer! I'll look into those classes and try to refactor the PR in this way.

[msridhar]

Thanks for the pointer! I'll look into those classes and try to refactor the PR in this way.

Thanks! Note that the directory structure doesn't matter here, i.e., it doesn't need to match the package names. So I'd create a folder nullaway/src/test/resources/com/uber/nullaway/testdata/spring-boot-annotations, put stub source files for the two annotations there (with the correct package name), and then for the tests, add them as additional source files, as is done here:

NullAway/nullaway/src/test/java/com/uber/nullaway/NullAwayAndroidTest.java

Lines 126 to 132 in 1ff88b6

	public void supportLibFragmentMissingOnCreateError() {
	initialiseAndroidCoreClasses();
	compilationHelper
	.addSourceFile("androidstubs/supportlib/Fragment.java")
	.addSourceFile("android-error/SupportLibraryFragmentWithoutOnCreate.java")
	.doTest();
	}

[ChristianCiach]

@uhafner Are you still working on it? We are having the same issue and are currently workarounding it by configuring NullAway with..

-XepOpt:NullAway:ExcludedFieldAnnotations=org.springframework.boot.test.mock.mockito.(Mock|Spy)Bean

I know nothing about the internal architecture of NullAway, but if you're busy I may give @msridhar suggestions a shot.

[msridhar]

Thanks for fixing this up! This now LGTM assuming tests pass

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: +0.08% 🎉

Comparison is base (70af259) 86.72% compared to head (e158336) 86.80%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #757      +/-   ##
============================================
+ Coverage     86.72%   86.80%   +0.08%     
  Complexity     1870     1870              
============================================
  Files            74       74              
  Lines          6178     6178              
  Branches       1202     1202              
============================================
+ Hits           5358     5363       +5     
+ Misses          408      405       -3     
+ Partials        412      410       -2     

Files Changed	Coverage Δ
...va/com/uber/nullaway/ErrorProneCLIFlagsConfig.java	87.20% <ø> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.