[code-reflection] RFR: More systematic binary op tests

[Hannes Greule]

On Sat, 16 Mar 2024 09:09:19 GMT, Hannes Greule <hgreule at openjdk.org> wrote:

> This is more of an experiment to gather some opinions. Please let me know what you think.
> 
> With this change, I'm trying to cover interpreter and bytecode generation/execution of binary ops in a more systematic approach.
> 
> I didn't find a good way to automatically *generate* test cases, especially as there is insufficient information about allowed types. Therefore, this approach consists of two main steps:
> 
> 1. A test that ensures that all bin ops have at least one test case
> 2. Dynamic tests based on these test cases
> 
> Note that these tests currently all fail. It seems like neither the interpreter nor the bytecode generator can deal with invokevirtual methods properly, so unboxing always fails.
> 
> ## Alternatives
> 
> One other idea I had was to, instead of going through the dynamic test cases, go through all methods in that class annotated with `@CodeReflection` and check this way if all bin ops are covered. This might allow for more extension to e.g. unary ops. The rest of the tests would then work as in other test classes. The problem of different data types and values would still exist though. Maybe a property-testing approach like `jqwik` would help with different values.

Thanks, that patch resolves the issue. I'll still wait for your opinion on the general structure before adding more test cases.

Thanks for your comments.

> There might be a way in JUnit to annotate the code reflected method as a test template and then annotate the test class with `@ExtendWith`, such that we extend JUnit to execute the code reflected method as we wish similarly to the dynamic approach, but without needing to reflectively enumerate all the methods under test.

I explored a bit how we can use JUnit extensions. One downside of directly annotating a `@CodeReflection` method as a test is that such methods need to be non-static (at least from what I've found out), which makes transforming more complicated. Alternatively, we could have one method that is a `@ParameterizedTest` with an argument provider that deals with the transformation and also provides the test input. I don't have something working for now, but that could look like:

    @ParameterizedTest
    @CodeReflectionSource
    void test(CoreOps.FuncOp funcOp, TestInput testInput, Result expectedResult) {
        Result interpret = runCatching(() -> interpret(testInput, funcOp));
        Result bytecode = runCatching(() -> bytecode(testInput, funcOp));
        assertResults(expectedResult, interpret);
        assertResults(interpret, bytecode);
    }

I think such a design could also be used for e.g. unary ops. The downside is that we still need to look for `@CodeReflection` methods ourselves. The upside is that it's clearer what the test is actually doing.

What's your opinion on that?

----

> Yes, my thinking was that function declaration operations don't return something that can be interacted with directly and cannot be passed around as values. We can however produce a symbol table of function name to function (e.g., for functions that are grouped into a module) and there is an operation to call a function by name (which will have stack effects). Functions are not intended to be part of some block of code and they do not capture values (we refer to the body as isolated). They are intended to model Java methods. Lambda and closure operations can be embedded in code and both can capture, the former returns an instance of a functional interface, and the latter returns an instance of a function type (which has no direct correlation to Java source).

That thinking absolutely makes sense. Maybe it's enough to add some docs, but currently I would expect others to also be confused by the difference between `resultType()` and `invokableType().returnType()`. Otherwise, splitting up `Op` into more interfaces would probably make sense, but that's something that most likely requires more exploration of the overall concept I guess.

I pushed a commit containing the approach using `@ParameterizedTest` in `CoreBinaryOpsTest`. I also included a bit more useful input values than before. I also adjusted some things to theoretically make it work with unary ops and test ops. The current implementation would also allow to use more specific types in the parameters of a parameterized test. I don't know if this is needed, but I could imagine that this could be helpful when e.g. wanting to make sure that floating point operations result in the same raw bits (e.g. using `Double#doubleToRawLongBits`).

Regarding the `@TestTemplate` approach, such methods don't only need to be non-static but also `void`, and the intention is to call exactly that method one or multiple times. So I think that's not what we want. `@Testable` is less restrictive, but from my understanding isn't picked up by the default junit test engine. Unless I missed something there, I think it's not worth further going into that direction.

I moved the relevant methods to the top. All the code surrounding the JUnit extension could be moved into separate files, allowing to use it in other test classes as well (but I'd need to figure out how that works with jtreg and find a good place for those files).

I also added code for other implemented binary ops now, as the test design is more stable now I guess.

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

PR Comment: https://git.openjdk.org/babylon/pull/40#issuecomment-2005004608
PR Comment: https://git.openjdk.org/babylon/pull/40#issuecomment-2023324571
PR Comment: https://git.openjdk.org/babylon/pull/40#issuecomment-2027221723
PR Comment: https://git.openjdk.org/babylon/pull/40#issuecomment-2041019594