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

Ensure consistent use of system properties in tests run by Gradle as well IDEA #74737

Closed
ywelsch opened this issue Jun 30, 2021 · 6 comments · Fixed by #75699
Closed

Ensure consistent use of system properties in tests run by Gradle as well IDEA #74737

ywelsch opened this issue Jun 30, 2021 · 6 comments · Fixed by #75699
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team

Comments

@ywelsch
Copy link
Contributor

ywelsch commented Jun 30, 2021

The Gradle test runner is using different system properties when running tests than what is used when tests are launched from the IDE (IntelliJ). This makes it tedious to run tests properly. Many security tests for example break because the system property es.set.netty.runtime.available.processors is not set (we set that for the Gradle test runner).

This provides a poor experience when running tests in the IDE, so I think that a fix should be prioritized accordingly.

@ywelsch ywelsch added >bug :Delivery/Build Build or test infrastructure needs:triage Requires assignment of a team area label labels Jun 30, 2021
@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Jun 30, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@DJRickyB DJRickyB removed the needs:triage Requires assignment of a team area label label Jun 30, 2021
@mark-vieira mark-vieira removed the >bug label Jun 30, 2021
@mark-vieira
Copy link
Contributor

mark-vieira commented Jun 30, 2021

This actually isn't a straitforward problem to solve. Unlike other settings which are applied to all tests, the system property you mention is configured explicitly on some test tasks for some projects. There's no mechanism in IntelliJ to convey this information to the IDE on a per-module basis. Realistically, the best way forward for stuff like this is to get to a position where we can delegate all test execution to Gradle vs using the platform test runner. Only that would guarantee a completely consistent experience.

@mark-vieira
Copy link
Contributor

Alternatively we could just set this globally for all tests. Would it be problematic to do so?

@ywelsch
Copy link
Contributor Author

ywelsch commented Jul 5, 2021

I don't think it should be problematic, but I would rather have it the other way around (not having to specify this property in Gradle nor IDEA): Given that this property (es.set.netty.runtime.available.processors) needs to be set whenever tests with the corresponding code are run is an indication that we got this backwards. Running the corresponding code in a JUnit context should disable the corresponding check by default (e.g. using a trick similar to what org.elasticsearch.common.Randomness employs to detect execution in a JUnit test context).

@mark-vieira
Copy link
Contributor

Running the corresponding code in a JUnit context should disable the corresponding check by default (e.g. using a trick similar to what org.elasticsearch.common.Randomness employs to detect execution in a JUnit test context).

I don't have super strong feelings on this but given this is "configuration" couldn't we just add something to ESTestCase to set this system property? Or something else that ES code subsequently looks for. This avoid hacky usages of reflection to detect a test execution context in production code.

@ywelsch
Copy link
Contributor Author

ywelsch commented Jul 26, 2021

That would work for me (there is already ESTestCase.setTestSysProps for this purpose).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants