HAI Profile overhaul #397
Merged
HAI Profile overhaul #397
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pitkni
approved these changes
Sep 1, 2023
Remove the profiles from Configuration and IntegrationTestConfiguration. Configuration is enabled by default, and we import IntegrationTestConfiguration explicitly when we use it. This frees us to use the test profile in all tests, especially integration tests. This means we can use application-test.properties for configuring the test environment. This on turn means we don't have to configure the main application.properties specifically for tests and we can use production-safe defaults.
corvidian
force-pushed
the
HAI/profile-overhaul
branch
from
b98718f to
3555473
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
corvidian
added a commit
that referenced
this pull request
Nov 20, 2023
Configure the Spring Context in integration tests to use a mockk instance of CableReportService so the mock can be `@Autowired`. CableReportService was defined in several integration tests as a `@MockkBean` dependency. The actual implementation was only used in `CableReportServiceITests`, which constructs its own custom instance. The other class that was injected with `@MockkBean` was `EmailSenderService`. Since there's an easy-to-use extension for mocking an actual email server, it felt better to use that instead of mocking the service universally. Also, unlike `CableReportServiceITests`, `EmailSenderServiceITest` uses the Spring context, so a context with the actual implementation is needed anyway. Each unique combination of `@MockkBeans` and `@SpykBeans` (or the Mockito equilevants) makes Spring create a new context for the test. While not a huge time sink, creating (and destroying) the contexts adds up. We can clearly see how many different contexts are created from the number of SQL connection pools that need to be shut down at the end of the tests. There are situations when they have to be used, but using alternative approaches is usually better. New contexts are also created whenever the configuration parameters are different between tests. This is sometimes unavoidable when we need to test how the application behaves with different configurations. Ever since #397 we've been able to use application.test.properties to configure the test environment, so we don't need to use test-specific properties to diverge from the production configuration. There was a properties parameter like remaining in `TestDataServiceITest`, so that was removed. There's one more service test with custom properties (`EmailSenderServicePropertiesITest`), but in that test we're interested in testing that the properties have the effect we want. And the properties are different from how we generally want to configure EmailSenderService in tests. So the custom properties are unavoidable. Controller tests are cheaper to create with varying configurations. Since only a very limited context is created for running them, recreating it is not very expensive. So testing things like feature flags on the controller level doesn't seem problematic.
3 tasks
corvidian
added a commit
that referenced
this pull request
Nov 24, 2023
Configure the Spring Context in integration tests to use a mockk instance of CableReportService so the mock can be `@Autowired`. CableReportService was defined in several integration tests as a `@MockkBean` dependency. The actual implementation was only used in `CableReportServiceITests`, which constructs its own custom instance. The other class that was injected with `@MockkBean` was `EmailSenderService`. Since there's an easy-to-use extension for mocking an actual email server, it felt better to use that instead of mocking the service universally. Also, unlike `CableReportServiceITests`, `EmailSenderServiceITest` uses the Spring context, so a context with the actual implementation is needed anyway. Each unique combination of `@MockkBeans` and `@SpykBeans` (or the Mockito equilevants) makes Spring create a new context for the test. While not a huge time sink, creating (and destroying) the contexts adds up. We can clearly see how many different contexts are created from the number of SQL connection pools that need to be shut down at the end of the tests. There are situations when they have to be used, but using alternative approaches is usually better. New contexts are also created whenever the configuration parameters are different between tests. This is sometimes unavoidable when we need to test how the application behaves with different configurations. Ever since #397 we've been able to use application.test.properties to configure the test environment, so we don't need to use test-specific properties to diverge from the production configuration. There was a properties parameter like remaining in `TestDataServiceITest`, so that was removed. There's one more service test with custom properties (`EmailSenderServicePropertiesITest`), but in that test we're interested in testing that the properties have the effect we want. And the properties are different from how we generally want to configure EmailSenderService in tests. So the custom properties are unavoidable. Controller tests are cheaper to create with varying configurations. Since only a very limited context is created for running them, recreating it is not very expensive. So testing things like feature flags on the controller level doesn't seem problematic.
corvidian
added a commit
that referenced
this pull request
Nov 24, 2023
Configure the Spring Context in integration tests to use a mockk instance of CableReportService so the mock can be `@Autowired`. CableReportService was defined in several integration tests as a `@MockkBean` dependency. The actual implementation was only used in `CableReportServiceITests`, which constructs its own custom instance. The other class that was injected with `@MockkBean` was `EmailSenderService`. Since there's an easy-to-use extension for mocking an actual email server, it felt better to use that instead of mocking the service universally. Also, unlike `CableReportServiceITests`, `EmailSenderServiceITest` uses the Spring context, so a context with the actual implementation is needed anyway. Each unique combination of `@MockkBeans` and `@SpykBeans` (or the Mockito equilevants) makes Spring create a new context for the test. While not a huge time sink, creating (and destroying) the contexts adds up. We can clearly see how many different contexts are created from the number of SQL connection pools that need to be shut down at the end of the tests. There are situations when they have to be used, but using alternative approaches is usually better. New contexts are also created whenever the configuration parameters are different between tests. This is sometimes unavoidable when we need to test how the application behaves with different configurations. Ever since #397 we've been able to use application.test.properties to configure the test environment, so we don't need to use test-specific properties to diverge from the production configuration. There was a properties parameter like remaining in `TestDataServiceITest`, so that was removed. There's one more service test with custom properties (`EmailSenderServicePropertiesITest`), but in that test we're interested in testing that the properties have the effect we want. And the properties are different from how we generally want to configure EmailSenderService in tests. So the custom properties are unavoidable. Controller tests are cheaper to create with varying configurations. Since only a very limited context is created for running them, recreating it is not very expensive. So testing things like feature flags on the controller level doesn't seem problematic.
corvidian
added a commit
that referenced
this pull request
Nov 24, 2023
Configure the Spring Context in integration tests to use a mockk instance of CableReportService so the mock can be `@Autowired`. CableReportService was defined in several integration tests as a `@MockkBean` dependency. The actual implementation was only used in `CableReportServiceITests`, which constructs its own custom instance. The other class that was injected with `@MockkBean` was `EmailSenderService`. Since there's an easy-to-use extension for mocking an actual email server, it felt better to use that instead of mocking the service universally. Also, unlike `CableReportServiceITests`, `EmailSenderServiceITest` uses the Spring context, so a context with the actual implementation is needed anyway. Each unique combination of `@MockkBeans` and `@SpykBeans` (or the Mockito equilevants) makes Spring create a new context for the test. While not a huge time sink, creating (and destroying) the contexts adds up. We can clearly see how many different contexts are created from the number of SQL connection pools that need to be shut down at the end of the tests. There are situations when they have to be used, but using alternative approaches is usually better. New contexts are also created whenever the configuration parameters are different between tests. This is sometimes unavoidable when we need to test how the application behaves with different configurations. Ever since #397 we've been able to use application.test.properties to configure the test environment, so we don't need to use test-specific properties to diverge from the production configuration. There was a properties parameter like remaining in `TestDataServiceITest`, so that was removed. There's one more service test with custom properties (`EmailSenderServicePropertiesITest`), but in that test we're interested in testing that the properties have the effect we want. And the properties are different from how we generally want to configure EmailSenderService in tests. So the custom properties are unavoidable. Controller tests are cheaper to create with varying configurations. Since only a very limited context is created for running them, recreating it is not very expensive. So testing things like feature flags on the controller level doesn't seem problematic.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Remove the profiles from Configuration and IntegrationTestConfiguration. Configuration is enabled by default, and we import IntegrationTestConfiguration explicitly when we use it.
This frees us to use the test profile in all tests, especially integration tests. This means we can use application-test.properties for configuring the test environment. This, in turn, means we don't have to configure the main application.properties specifically for tests and we can use production-safe defaults.
Jira Issue: -
Type of change
Checklist:
or other location: