RFR: 8369232: testlibrary_tests/ir_framework/tests/TestScenariosCrossProduct.java timed out [v4]

[Damon Fenacci]

On Tue, 14 Oct 2025 17:09:45 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> The test ` testlibrary_tests/ir_framework/tests/TestScenariosCrossProduct.java` intermittently timed out in our CI. On my local machine, I measured ~80s which is quite long given that we actually only want to test that Cartesian product for scenarios work.
>> 
>> #### Reduce Execution Time by not Executing the Scenarios
>> I had a closer look at the test to try to cut the execution time down. Currently, we are executing many IR framework runs with different Cartesian products for scenarios. Afterwards, we compare the output of IR matching to check if the computation of the Cartesian products were correct. However, since we are only interested in verifying that the Cartesian product computation for scenarios works (i.e. `addCrossProductScenarios()`), we could skip the actual execution of the scenarios itself - we can trust that the IR framework is already tested well enough.
>> 
>> To achieve that, we can use reflection to get the added scenarios to the IR framework (I don't want to add a public accessor because a user should not need access to them) and then fetch the corresponding scenario flags and compare against our expectation. That's what I propose with this change.
>> 
>> #### Changes
>> - Verification without actually running scenarios.
>> - Added a test passing 3 sets to `addCrossProductScenarios()` which was missing before.
>> - Improved `addScenarios()` where we added a scenario to the list even though it already existed. That normally does not matter because we are throwing a `TestFormatException` anyway afterwards. But it messes with the test: We are adding the duplicated scenario and then read it again in the verification part of the test.
>> - Refactored the test a little more.
>> - Refactored some small things in `addCrossProductScenarios()` while looking at it.
>> - Added a sentence about passing a single set to `addCrossProductScenarios()` which was not evidently clear what is happening when looking at the method comment.
>> 
>> #### Execution Time Comparison
>> Measured on my local machine:
>> - Mainline: ~80s
>> - With patch: ~2-3s
>> 
>> Thanks,
>> Christian
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review Emanuel

Looks good to me otherwise. Thanks @chhagedorn.

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestScenariosCrossProduct.java line 337:

> 335:             Asserts.assertTrue(stdErr.contains("Scenario flags: [-XX:-UseNewCode, -XX:-UseNewCode2]"));
> 336:             Asserts.assertTrue(stdErr.contains("Scenario flags: [-XX:+UseNewCode, -XX:-UseNewCode2]"));
> 337:             Asserts.assertTrue(stdErr.contains("Scenario flags: [-XX:-UseNewCode, -XX:+UseNewCode2]"));

This might be partially redundant with the full stop in the first assert above but maybe it would be worth checking that we don't have any additional "Scenario flags:..." string.

-------------

Marked as reviewed by dfenacci (Committer).

PR Review: https://git.openjdk.org/jdk/pull/27672#pullrequestreview-3340205271
PR Review Comment: https://git.openjdk.org/jdk/pull/27672#discussion_r2432430965