RFR: 8254129: IR Test Framework to support regex-based matching on the IR in JTreg compiler tests [v2]

[Igor Ignatyev]

On Mon, 19 Apr 2021 08:10:13 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> test/lib/jdk/test/lib/hotspot/ir_framework/IRMatcher.java line 116:
>> 
>>> 114:     private void parseHotspotPidFile() {
>>> 115:         Map<Integer, String> compileIdMap = new HashMap<>();
>>> 116:         try (BufferedReader br = new BufferedReader(new FileReader(new File(System.getProperty("user.dir") + File.separator + hotspotPidFileName)))) {
>> 
>> more idiomatic/modern version would be
>> Suggestion:
>> 
>> try (BufferedReader br = Files.newBufferedReader(Paths.get(System.getProperty("user.dir"), hotspotPidFileName))) {
>> 
>> 
>> I actually not sure if you really need `$user.dir`, won't hotspot_pid file get generated in the current dir?
>
> Yes it should. Would this be equivalent to `Paths.get("")`? It seems to work like that.

`Paths.get(hotspotPidFileName)` returns you a `Path` to `hotspotPidFileName` in cwd.

>> test/lib/jdk/test/lib/hotspot/ir_framework/TestFramework.java line 958:
>> 
>>> 956:         throw new TestFrameworkException("Internal Test Framework exception - please file a bug:\n" + failureMessage, e);
>>> 957:     }
>>> 958: }
>> 
>> I am not convinced that we really these guys when we already have `TestFormat::check` and `TestRun::check` (I'm actually not 100% convinced that we need check/fail in both TestFormat and TestRun)
>
> I wanted to distinguish the following cases (but I'm open to use a different approach):
> 
> - `TestFormat::check` (using `TestFormatException`): The format of any test/helper class does not conform with the format the framework is expecting. The user needs to fix this (for example, using a wrong annotation). I report these kind of failures before actually running any of the tests [here](https://github.com/openjdk/jdk/blob/7ed789dc98691d7c7fc2295e045a9f54b9fa6277/test/lib/jdk/test/lib/hotspot/ir_framework/TestFrameworkExecution.java#L265).
> - `TestRun::check` (using `TestRunException`): Thrown while the framework is [executing a test](https://github.com/openjdk/jdk/blob/7ed789dc98691d7c7fc2295e045a9f54b9fa6277/test/lib/jdk/test/lib/hotspot/ir_framework/TestFrameworkExecution.java#L799), there is nothing wrong with the format of a test.
> - `TestFramework::check` (using `TestFrameworkException`): If such a check fails there is a bug in the framework itself (like internal framework assertions). The user cannot really do anything about it without fixing the framework itself.
> 
> I have to double check again if I'm using the right kind of exception everywhere.

I understand that you want to use different exception types to distinguish different kinds of failures, I just don't see lots of benefits in have `::check` and `::fail` methods, they are just `if (p) throw new X(m)` or `throw new X(m)`.

if, for example, you want `TestFrameworkException` to always have `Internal Test Framework exception - please file a bug`, you can prepend it in `TestFrameworkException::<init>`

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

PR: https://git.openjdk.java.net/jdk/pull/3508