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

[Christian Hagedorn]

On Tue, 14 Oct 2025 08:46:23 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8369232
>  - add missing test
>  - 8369236: testlibrary_tests/ir_framework/tests/TestScenariosCrossProduct.java timed out

After discussing it further offline (thanks for the discussion!), you convinced me that we should at least add a sanity end-to-end test to verify the `TestFramework.addCrossProductScenarios()` is properly working. I added such a test but removed the reliance on IR matching with flag constraints matching. I instead parse the stderr output to search for the used scenario flags.

I also pushed an update about the `Set::copyOf` comment further up.

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

PR Comment: https://git.openjdk.org/jdk/pull/27672#issuecomment-3402467737