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

test: use ContextConfiguration instead of profiles for DB DHIS2-17821 #18270

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

teleivo
Copy link
Contributor

@teleivo teleivo commented Aug 1, 2024

Our production code has logic in it that changes its behavior on whether its running inside a test or with a given test profile.

This is risky as we might not run some crucial logic if we don't test this well. Spring has other ways for us to override part of our application during testing that does not compromise production code.

We configure the Spring (web) ApplicationContext using @ContextConfiguration in our tests. These can be inherited, merged and overridden. The inheritance can also be opted-out of. Using this we can override specific beans like the dhisConfigurationProvider so that we test against H2 or Postgres with Minio.

Our production code does a @ComponentScan so Spring creates all components (config, service, repo).

So far the IntegrationTestBaseConfig did a @ComponentScan like we do in production code of org.hisp.dhis code. This includes all our test @Configuration classes during integration testing.

I found an issue when I made following improvement:

I moved H2 related beans into the H2Configuration class. H2 does not support JSON, so we need to register fake functions with the data source. We also use H2 auto DDL feature instead of running flyway.

When I ran a PostgresIntegrationTest it was not running the flyway migrations. The reason was that it used the NoOpFlyway from the H2Configuration.

Summarized version of Spring logs during a PostgresIntegrationTest

Overriding bean definition for bean 'dhisConfigurationProvider' with a different definition:
replacing `org.hisp.dhis.external.conf.DefaultDhisConfigurationProvider`
with `org/hisp/dhis/test/config/H2DhisConfiguration.class`

Overriding bean definition for bean 'flyway' with a different definition:
replacing flyway `org/hisp/dhis/db/migration/config/FlywayConfig.class`
with `org/hisp/dhis/test/config/H2DhisConfiguration.class`

Overriding bean definition for bean 'namedParameterJdbcTemplate' with a different definition:
replacing `org/hisp/dhis/config/DataSourceConfig.class`
with `org/hisp/dhis/test/config/H2DhisConfiguration.class`

Overriding bean definition for bean 'dataSource' with a different definition:
replacing `org/hisp/dhis/config/DataSourceConfig.class`
with `org/hisp/dhis/test/config/H2DhisConfiguration.class`

Overriding bean definition for bean 'dhisConfigurationProvider' with a different definition:
replacing `org/hisp/dhis/test/config/H2DhisConfiguration.class`
with `org/hisp/dhis/test/config/MinIOConfiguration.class`

Overriding bean definition for bean 'dhisConfigurationProvider' with a different definition:
replacing `org/hisp/dhis/test/config/MinIOConfiguration.class`
with `org.hisp.dhis.test.config.PostgresDhisConfiguration`

So you can see that individual beans from the prod config are overridden by our test configs. You can also see that even though its a Postgres test the H2Configuration does override the flyway bean. That explains why no migrations are run even though we do setup a Postgres DB in the end.

To be really explicit and in control we can filter out all test related @Configurations with excludeFilters = @ComponentScan.Filter(type = FilterType.REGEX, pattern = "org\\.hisp\\.dhis\\.test\\..*"). Then include only the ones we need using a @ContextConfiguration.

After this filter is applied the bean creation is much simpler

Overriding bean definition for bean 'dhisConfigurationProvider' with a different definition:
replacing `org.hisp.dhis.external.conf.DefaultDhisConfigurationProvider`
with `org.hisp.dhis.test.config.PostgresDhisConfiguration`

Spring Boot has a https://reflectoring.io/spring-boot-testconfiguration/
which is a @Configuration that is not picked up by @ComponentScan. It has to be explicitly be imported in a test. The approach we take here is similar.

Related articles

https://reflectoring.io/dont-use-spring-profile-annotation/
https://reflectoring.io/spring-boot-testconfiguration/

Debugging Bean Creation

If you ever need to debug what beans get created enable this one

        <Logger name="org.springframework.beans.factory.support.DefaultListableBeanFactory" level="debug" additivity="false">
            <AppenderRef ref="console"/>
        </Logger>

in dhis-2/dhis-test-integration/src/test/resources/log4j2-test.xml for integration tests
in dhis-2/dhis-support/dhis-support-test/src/main/resources/log4j2-test.xml for other tests

@teleivo teleivo force-pushed the DHIS2-17821-db-profiles branch 5 times, most recently from 7834a2d to 9278ae4 Compare August 2, 2024 12:09
@teleivo teleivo force-pushed the DHIS2-17821-db-profiles branch 2 times, most recently from 3efb2c8 to d0faf19 Compare August 27, 2024 12:27
@teleivo teleivo marked this pull request as ready for review August 27, 2024 13:09
Copy link

sonarcloud bot commented Aug 27, 2024

@IntegrationH2Test
@ActiveProfiles("test-h2")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this profile still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we still have production code that is aware of profiles like

. We might be able to solve this particular case. I have not tried.

I tried to get rid of prod code knowing about test profiles in general in #18282 but am blocked. The issue is that some tests need to set specific dhis.conf properties. We do this via @ContextConfiguration overriding beans. We use @Conditional like

to create certain components only if a feature is active. So there is a chicken and egg situation 😅 Spring Boot has automatic support for what we would like to do with application.properties and ConditionalOnProperty. It would be amazing if we can come up with something similar in https://dhis2.atlassian.net/browse/DHIS2-17797. @jbee let's talk about this when you are ready to look into https://dhis2.atlassian.net/browse/DHIS2-17797.

@teleivo teleivo merged commit c22fe39 into master Aug 28, 2024
15 checks passed
@teleivo teleivo deleted the DHIS2-17821-db-profiles branch August 28, 2024 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants