Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strip ### from tests #260

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Conversation

stepancheg
Copy link
Contributor

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.

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
Copy link
Contributor

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
Copy link
Contributor Author

stepancheg commented Aug 8, 2023

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:

  • golden tests: these would need frequent regeneration of golden files, but that would be mostly mechanical
  • error classifier. Test runner would detect two classes of errors: parsing or evaluation. And tests would not check specific messages, but only a class of error
if: ### PARSE

---

().remove ### EVALUATION

@laurentlb
Copy link
Contributor

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.

@laurentlb laurentlb merged commit 5704ba4 into bazelbuild:master Aug 8, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants