bazelbuild / starlark

Starlark Language
Apache License 2.0
2.38k stars 158 forks source link

Strip ### from tests #260

Closed stepancheg closed 11 months ago

stepancheg commented 11 months ago

So we actually test error messages, not source code: both rust and java includes snippet in error message, so before this commit error messages were not tested on java and rust.

This commit is probably the most wasteful time I spent in the last year. Starting to doubt these tests do more good than harm.

laurentlb commented 11 months ago

Thanks! (and sorry that you wasted time on this)

Why did you split lots of error messages with 3 values java:/go:/rust:? The original idea is that we didn't care which interpreter produces which error message, as long as it produces some related error message. e.g. I'd rather not have the test failing if the Java implementation starts using the same message as the Rust implementation.

stepancheg commented 11 months ago

Thanks! (and sorry that you wasted time on this)

No problem.

Why did you split lots of error messages with 3 values

Otherwise it's hard to fix tests: I see a java test failing for Java and Rust for example, but I don't know which two of three patterns I need to fix.

I'd rather not have the test failing if the Java implementation starts using the same message as the Rust implementation.

It is more likely that Java error message will change to something else that is neither Rust, nor Go.

But I can easily merge these threes back into "or" patterns with a script.

If we want to keep these tests, I think of two alternatives:

if: ### PARSE

---

().remove ### EVALUATION
laurentlb commented 11 months ago

The "error classifier" approach sounds good to me. I'd like to avoid updating this repo when an interpreter changes.

I'm merging this PR already, feel free to send a new one if you want to simplify the tests.