Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[inject-test] Can inject the BeanScope into @InjectTest tests. #758

Closed
wants to merge 1 commit into from

Conversation

SentryMan
Copy link
Collaborator

Now tests using @InjectTest are able to inject the full BeanScope into the test.

@SentryMan SentryMan added the enhancement New feature or request label Jan 10, 2025
@SentryMan SentryMan added this to the 11.2 milestone Jan 10, 2025
@SentryMan SentryMan self-assigned this Jan 10, 2025
@SentryMan SentryMan requested a review from rbygrave January 10, 2025 04:07
@SentryMan SentryMan enabled auto-merge January 10, 2025 04:07
@rbygrave
Copy link
Contributor

What is the motivation for this?

BeanScope in component testing has parent/child "layers" and the appropriate BeanScope for a given test depends on what components are injected. So to me this kind of breaks that determination because injecting BeanScope means ANY dependency can be used in the test.

So why is this needed?

@SentryMan
Copy link
Collaborator Author

SentryMan commented Jan 10, 2025

Had a guy on the discord that needed to do something like have test beans that also have dependencies on beans from src/main to be used for multiple test classes.

Naturally this is currently doesn't fly, so the next best thing is having a utility class that accepts certain beans as parameters.

This helps reduce the amount of boilerplate required per test as the utility class can extract all the beans it requires.

@SentryMan
Copy link
Collaborator Author

It made sense to me as we can inject the beanscope into regular beans, so why should tests be the exception?

@ferrazoli
Copy link

Hi, I'm the guy on Discord that @SentryMan mentioned. Thank you so much for the suggestions on Discord and for this PR. ❤️

For functional/integration tests this would be really useful. The immediate use case for myself would be some sort of TestEntityFactory that depends on a UserService that I use to create new users. While I could inject the UserService directly, the TestEntityFactory has some additional functionality, such as generating randomized email addresses that I input to my UserService.

As far as I understand it, the DI lifecycle in tests has two steps:

  1. @TestScope, or test beans are wired to each other. Because this is the first step, they cannot depend on any of the beans in main.
  2. main beans are wired to each other. If a main bean depends on a bean that was already initialized during the first step, that bean is used and the main bean is not initialized.

This makes sense to me, but it would be really useful if the wiring was smart enough to understand that:

  • A test bean that depends on a bean that isn't available in the TestBeanScope can try to wire it to the main bean. To avoid mistakes, I think it would be a good idea to log a warning when this happens.
  • Alternatively, it would be useful to have a "third scope" that I can use to wire test beans to main beans after step 2.

On top of this, I can think of many potential use cases where wiring a @TestScope bean that depends on main beans would be also useful. For instance, if I have a dependency chain UserController -> AuthService -> UserRepository, maybe I want to inject a custom AuthService bean that doesn't interact with third party APIs without having to also provide a test scoped UserRepository, but currently Avaje doesn't give me a simplified way of doing this. I can only @TestScope beans that have no dependencies, or I have to provide all of their dependencies in the @TestScope as well.

As SentryMan explained, my current workaround for this is to make my TestEntityFactory a sort of utility class that takes a UserService as a parameter on all of its methods, or manually instantiate it on every test, which adds a lot of boilerplate.

I'm not familiar enough with the codebase to comment on whether this PR solves the issue for me, but I wanted to pitch in on the motivation behind my requests on Discord.

I'm also open to the possibility that I could be writing awful tests. 😅 If that's the case, I would appreciate it if you can nudge me in the right direction.

@rbygrave
Copy link
Contributor

rbygrave commented Jan 10, 2025

So wrt ...

UserController -> AuthService -> UserRepository, maybe I want to inject a custom AuthService

You can inject a mock / test double for AuthService in a @InjectTest. When the test includes @Mock AuthService or @Spy AuthService then that is the instance that will be used for that test.

currently Avaje doesn't give me a simplified way of doing this

Ah, just use @Mock or @Spy ... perhaps the docs are not great on that point at: https://avaje.io/inject/#inject-test

Am I misunderstanding?

As far as I understand it, the DI lifecycle in tests has two steps:

Yes, that's basically correct.

  • The @TestScope is internally called "testBaseScope"
  • The main ... is internally called "testAllScope"

Which BeanScope is used for a given test depends on what beans are injected into the test [as test doubles / mocks].

If there are no mocks/test doubles ... the test can use the "testAllScope", otherwise a new BeanScope needs to be created for that test, and it uses the "testBaseScope" as its parent - so all the @TestScope beans are still used but everything else is created/wired as a new component [apart from the test doubles that the test is using].

Now this is ok but not ideal in that some components are way more expensive than others - datasources, repositories, databases etc. That is, there is some improvement that I have been thinking about that could be made where we have another "testRepoScope" that contains the relatively expensive components (dataSource etc). In this way tests that provide test doubles don't need to re-wire those more expensive components. This "testRepoScope" would sit between the "testAllScope" and "testBaseScope".

It would then be:

  • No mocks / test doubles for this test ... use "testAllScope"
  • Some mocks, but no test doubles in "testRepoScope" ... we still use "testRepoScope" to create the new BeanScope
  • Some mocks, including test doubles in "testRepoScope" ... we still use "testBaseScope" to create the new BeanScope

... and we'd do this because of the performance benefit, we have enough tests that use mocks/test doubles + leading to the need to wire "relatively expensive" components for each test.

I'll suggest that this part is more a side topic to the issue though?

@ferrazoli
Copy link

@rbygrave I spoke to SentryMan on Discord about this.

As SentryMan explained, my current workaround for this is to make my TestEntityFactory a sort of utility class that takes a UserService as a parameter on all of its methods, or manually instantiate it on every test, which adds a lot of boilerplate.

This PR helps with this workaround because assuming TestEntityFactory depends on multiple beans, e.g. UserService, TagService, ProductService, etc, then I would have to wire all of those beans in all my tests and pass them all as parameters, which adds a lot of boilerplate. If I can access the full BeanScope from the test, I can instead just pass that as a parameter, saving a lot of boilerplate.

@ferrazoli
Copy link

I'll suggest that this part is more a side topic to the issue though?

Correct. I misunderstood the goal of this PR until I spoke to SentryMan and was just going based off what I was asking on Discord.

Ah, just use @mock or @SPY ... perhaps the docs are not great on that point at: https://avaje.io/inject/#inject-test
Am I misunderstanding?

I don't want to use @Mock because I have an actual implementation. As for @Spy, I'm not sure how it helps as in this scenario my spy depends on UserRepository which won't be available at that time, correct?

@rbygrave
Copy link
Contributor

I don't want to use @mock because I have an actual implementation.

That is a documentation failure on my part. For an @InjectTest test with a @Mock field that has an implementation [is not null] is going to be the test double that is put into the BeanScope.

That is, a @Mock that is null becomes a Mockito mock, but if it isn't null [it has an implementation already] then that implementation is used in the test.

For example:

@Mock HelloData mockData = () -> "ImAConcreteTestDouble";
@Mock HelloData mockData = initMe();

  private HelloData initMe() {
    // whatever code we want to create the test double
    ...
    return helloData;
  }

@ferrazoli
Copy link

Interesting. But I still don't see how that solves the issue. Your example misses one of my issues which is the fact that the initMe method would take a bean from BeanScope as a parameter, but during the time of its initialization, this bean is not available.

In Discord I also noticed another issue where this PR would help. It seems that I can't inject List beans with @Inject on tests. SentryMan suggested programmatic testing such as:

@Test
void test() {

  try (BeanScope context = TestBeanScope.builder()
    .forTesting()
    .build()) {

    List<AvajeJavalinPlugin> webRoutes = context.list(AvajeJavalinPlugin.class);
    //whatever else
  }
}

But again, this is a lot of boilerplate. If I could inject the BeanScope into the test, I could access it from a @BeforeEach and reduce the amount of boilerplate by extracting the list once at that time.

@rbygrave
Copy link
Contributor

rbygrave commented Jan 12, 2025

misses one of my issues which is the fact that the initMe method would take a bean from BeanScope as a parameter

So the use case has a test double for AuthService, and this test double wants to make use of the real/injected UserRepository [say in order to assist setting up test data or something like that].

Well, normally no you can't pass the UserRepository into the constructor of the test double but we can indeed set it up post construction say in @BeforeEach. Is there are reason that isn't sufficient for this case? Can you explain more how the test double for AuthService is needing/using the UserRepository?

@ferrazoli
Copy link

@SentryMan @rbygrave

The list issue was fixed in #761 and the injecting issue was fixed in #760. I think this PR can be closed. Thank you both for all the help!

@SentryMan SentryMan closed this Jan 13, 2025
auto-merge was automatically disabled January 13, 2025 21:43

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants