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

[Emanuel Peter]

On Tue, 14 Oct 2025 15:42:11 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:
> 
>   add simple end to end test + review comment

Nice, thanks for the updates. Looks good to me now :)

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

> 264:         private static void assertSameResultWhenManuallyAdding(List<Scenario> scenariosFromCrossProduct,
> 265:                                                                Set<Set<String>> expectedScenariosWithFlags) {
> 266:             List<Scenario> expectedScenarios = getScenariosWIthFlags(expectedScenariosWithFlags);

Suggestion:

            List<Scenario> expectedScenarios = getScenariosWithFlags(expectedScenariosWithFlags);

typo?

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

> 269:         }
> 270: 
> 271:         private static List<Scenario> getScenariosWIthFlags(Set<Set<String>> expectedScenariosWithFlags) {

Suggestion:

        private static List<Scenario> getScenariosWithFlags(Set<Set<String>> expectedScenariosWithFlags) {

looks like a typo

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

> 330:             // Expected.
> 331:             System.setErr(originalErr);
> 332:             Asserts.assertTrue(e.getMessage().contains("The following scenarios have failed: #0, #1, #2, #3"));

Can you somehow check that there is nothing after the `#3`? Just to make sure we don't have a `#4` ;)

Optional if too much work.

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

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27672#pullrequestreview-3336331741
PR Review Comment: https://git.openjdk.org/jdk/pull/27672#discussion_r2429647560
PR Review Comment: https://git.openjdk.org/jdk/pull/27672#discussion_r2429646743
PR Review Comment: https://git.openjdk.org/jdk/pull/27672#discussion_r2429651439