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

Improve @SpringBatchTest to autowire the job under test in JobLauncherTestUtils if it is unique #4218

Closed
wants to merge 2 commits into from

Conversation

hpoettker
Copy link
Contributor

To resolve #1237, the feature to always autowire a job into JobLauncherTestUtils was removed as the autowiring leads to issues if there is no unique candidate in the test context.

This PR adds the flag autowireJob to the annotation @SpringBatchTest. If set to true, the developer opts in to the old behavior.

Using narrowly defined test contexts when writing unit tests for jobs is a good practice in my opinion. Similar to using test slices for a Spring Boot application, it can have a strong influence on the run-time of build pipelines and thus tighten feedback cycles during development.

The proposed change is intended to be an incentive for test contexts that contain only the job to be tested. Additionally, it simplifies migration to Spring Batch 5 a bit for developers that relied on the old behavior.

@fmbenhassine
Copy link
Contributor

To resolve #1237, the feature to always autowire a job into JobLauncherTestUtils was removed as the autowiring leads to issues if there is no unique candidate in the test context.

We had an internal discussion about this, and one of the ideas that came out to address the case where multiple jobs are defined in the test context was to add an attribute with the name of the job to autowire in JobLauncherTestUtils. We decided to not implement this feature as such an information is statically defined in an annotation (and would also "encourage" a bad practice that we want avoid in the first place).

Using narrowly defined test contexts when writing unit tests for jobs is a good practice in my opinion.

I couldn't agree more! See my comment here: #3699 (comment)

The proposed change is intended to be an incentive for test contexts that contain only the job to be tested. Additionally, it simplifies migration to Spring Batch 5 a bit for developers that relied on the old behavior.

For the case where a single job is defined, I think there is a room for improvement with a sensible default. What you suggest is a good idea IMO. However, do we really need a flag for that? What about autowiring the job in JobLauncherTestUtils if there is only one? This removes the need for a flag and is nearly the same behaviour as in v4. wdyt?

@fmbenhassine fmbenhassine added pr-for: enhancement in: test status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter labels Oct 24, 2022
@hpoettker
Copy link
Contributor Author

Thanks for the feedback! I've implemented the proposal to only inject a job into JobLauncherTestUtils if it's unique.

For me personally this solution would work very well. I'm wondering however if it's not too "magical" for developers who are not aware of the underlying mechanism. To them it might seem that the injection process is flaky as "sometimes" a job is injected and "sometimes" not.

The original commit with the flag has the advantage that it's clearer how it works: If the flag is not set, then no job will be injected. Otherwise, a job will be injected or the test fails while trying to do so.

@fmbenhassine
Copy link
Contributor

Thank you for your feedback and for the quick update!

For me personally this solution would work very well.

I believe this is because you are applying the best practice of isolating tests for each job. And this would work very well too for anyone who is applying this best practice. My thinking with the suggested default is as follows: It is unfair to make users who apply the best practice (of isolating tests) to pay the price of autowiring the single job in test utilities.

I'm wondering however if it's not too "magical" for developers who are not aware of the underlying mechanism. To them it might seem that the injection process is flaky as "sometimes" a job is injected and "sometimes" not.

There is no magic. Magical is only magical if it is not documented. I will refer anyone who would complain about this being magical to this tweet by Josh 😄 . That said, I see no note about this new default in the Javadoc of SpringBatchTest in this PR. I will take care of that on merge.

Thanks again for all your contributions! REALLY appreciated 👍

@fmbenhassine fmbenhassine removed the status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter label Oct 27, 2022
@fmbenhassine fmbenhassine added this to the 5.0.0-RC2 milestone Oct 27, 2022
@fmbenhassine fmbenhassine changed the title Add flag to @SpringBatchTest to autowire a job bean Improve @SpringBatchTest to autowire the job under test in JobLauncherTestUtils if it is unique Oct 27, 2022
fmbenhassine pushed a commit that referenced this pull request Nov 8, 2022
fmbenhassine added a commit that referenced this pull request Nov 8, 2022
* Update Javadoc of SpringBatchTest about job autowiring
* Apply Spring code style conventions
@fmbenhassine
Copy link
Contributor

Rebased and merged as 3bb7ce5. Revised in 1527593. Thank you for your contribution!

@hpoettker hpoettker deleted the autowire-job branch November 8, 2022 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The job under test should not be autowired in JobLauncherTestUtils
2 participants