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

Apply test config profile (%test properties) for runtime properties when running integration tests (native tests) #26279

Closed
yrodiere opened this issue Jun 22, 2022 · 9 comments
Labels
area/config area/core area/testing kind/enhancement New feature or request triage/duplicate This issue or pull request already exists

Comments

@yrodiere
Copy link
Member

yrodiere commented Jun 22, 2022

Description

When running integration tests (@QuarkusIntegrationTest), we execute tests against a native build of the application, one that was built with the prod configuration profile. The main reason for that is that a native build takes a lot of time, and doing two native builds of the application (once with the prod profile, once with the test profile) would add unreasonable overhead.

This sometimes leads to confusing behavior, e.g. some configuration properties such as %test.quarkus.hibernate-orm.sql-load-script or %test.quarkus.hibernate-search-orm.automatic-indexing.synchronization.strategy being ignored in native integration tests.

While I think this limitation is acceptable for build-time properties, I also think we would get much better user experience by at least applying runtime %test properties when running (native) integration tests, as most people probably expect.

Technically it should be doable as it wouldn't require to re-build the application, and it would certainly make configuration more intuitive.

While we're at it, we could even consider displaying warnings when starting the (native) integration tests if the developer tried to set build-time properties in the %test profile.

Implementation ideas

No response

@geoand
Copy link
Contributor

geoand commented Jun 22, 2022

We currently have the (very incorrectly named quarkus.test.native-image-profile) property which allows to control the profile the application is launched in.

In my view, we should simply add quarkus.test.intergration-test-profile and make things very clear in the documentation

@yrodiere
Copy link
Member Author

In my view, we should simply add quarkus.test.intergration-test-profile and make things very clear in the documentation

And probably have it default to test, then? Otherwise I fear we'll keep confusing people... Documentation is nice, but "instinctive" behavior is better.

@geoand
Copy link
Contributor

geoand commented Jun 22, 2022

And probably have it default to test, then?

That's where I disagree :)

@yrodiere
Copy link
Member Author

yrodiere commented Jun 22, 2022

I'm just guessing since you didn't give any details, but if your point is that integration tests should use different configuration than other tests, then at the very least we'd need quarkus.test.integration-test-profile to default to a dedicated profile, e.g. integrationtest. Together with #26283, I suppose people would then be able to configure both tests and integration tests easily (e.g. %[test,integrationtest,dev].quarkus.datasource.jdbc.url=jdbc:postgresql://localhost/quarkus_test).

I mean, it stands to reason that people don't execute their integration tests in their production environment. So at the very least, we can say that it (almost) never makes sense to use the prod profile when running integration tests [EDIT: for runtime properties, at least]. So the default will always need to be overridden... so it's a bad default?

@geoand
Copy link
Contributor

geoand commented Jun 22, 2022

I don't agree that it's bad default, although I understand why people (perhaps even most?) would think so.

The way I see it, the integration test should do exactly the same thing as the application run in production, unless otherwise requested.
As for the argument that people would need to use prod configuration, well I'm totally fine with that because people should never have real production configuration in application.properties anyway.

@yrodiere
Copy link
Member Author

The way I see it, the integration test should do exactly the same thing as the application run in production, unless otherwise requested.

While I agree that's the point of an integration test, from what I can see we don't really use @QuarkusIntegrationTest like this. In Quarkus and quickstarts at least, we generally have a JVM test using @QuarkusTest, and a native test that extends it and uses @QuarkusIntegrationTest. So we basically use @QuarkusIntegrationTest, not to actually run... integration tests... but just to run usual tests with a native image.

An example. Tests would likely involve executing multiple operations in sequence. In the case of Hibernate Search, this probably means indexing a few entities, then performing a search to see whether the newly indexed entities show up in search results. Except, with the default settings that we recommend to use in production, indexing is asynchronous, so your test will probably fail randomly (the newly indexed entities won't show up immediately in search). Which is why we recommend setting quarkus.hibernate-search-orm.automatic-indexing.synchronization.strategy = sync for tests: it's forces synchronous indexing, which is just more convenient for tests.

So yeah... integration tests should use settings as close to production as possible... but sometimes it just doesn't make sense. Integration tests are not executing in a production environment, so there are differences between integration tests and production, and currently, it's hard to reflect that in configuration.

I get why you wouldn't want to use a test profile for integration tests, but at the very least an integrationtest profile (maybe with prod as a parent) seems to make sense to me.

As for the argument that people would need to use prod configuration, well I'm totally fine with that because people should never have real production configuration in application.properties anyway.

In that case, consider properties that would usually be set in integration tests, but not in prod (because the default is fine for prod, which makes sense, right?). For example quarkus.hibernate-orm.database.generation (none, the default, is fine for prod, but you may want drop-and-create for ITs), or quarkus.hibernate-search-orm.automatic-indexing.synchronization.strategy (write-sync is best for prod, but you absolutely need sync for sequential tests).

If people were to set these properties in the prod profile, they would have to take extra care to override them in their .env... to reset them to their default value.

So, the current solution (the one we use in our documentation, at least) is generally to set the property without any profile, and to reset it with the prod profile. It really feels backwards, and more importantly: dangerous. Imagine forgetting to override quarkus.hibernate-orm.database.generation=drop-and-create in your prod profile? And in a slightly less dramatic way, I'm sure many people forgot to reset quarkus.hibernate-search-orm.automatic-indexing.synchronization.strategy in their prod profile, leading to very bad performance as soon as they get a significant load on their application.

@geoand
Copy link
Contributor

geoand commented Jun 22, 2022

Yeah, I understand the reasoning for those specific properties, although I still don't think it's worth changing the existing behavior.

In any case, if someone makes the change, I personally won't block it, but I won't be doing the work myself (although I can provide pointers to what needs to be changed).

@geoand
Copy link
Contributor

geoand commented Jul 15, 2022

I am going to close this in favor of #24581 which we had discussed at some point.

@geoand geoand closed this as not planned Won't fix, can't repro, duplicate, stale Jul 15, 2022
@geoand geoand added the triage/duplicate This issue or pull request already exists label Jul 15, 2022
@yrodiere
Copy link
Member Author

I doubt this solution will do much to reduce confusion from users who set a configuration property in the "test" profile and don't see it applied when running tests.

But as I stated above, I agree it's a start:

I'm just guessing since you didn't give any details, but if your point is that integration tests should use different configuration than other tests, then at the very least we'd need quarkus.test.integration-test-profile to default to a dedicated profile, e.g. integrationtest. Together with #26283, I suppose people would then be able to configure both tests and integration tests easily (e.g. %[test,integrationtest,dev].quarkus.datasource.jdbc.url=jdbc:postgresql://localhost/quarkus_test).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config area/core area/testing kind/enhancement New feature or request triage/duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants