-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Better error array numerics #6420
Conversation
Benchmark for a0deb0bClick to view benchmark
|
9e149c6
to
7332e18
Compare
Benchmark for 4c3ff5dClick to view benchmark
|
Benchmark for 8ee81ccClick to view benchmark
|
+1 for libtest mimic, we've been using it over in the fuel-core repo already as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that we are starting with snapshot tests!! 💯 🥇 Can we just add to the README.md of the e2e_vm_tests
short instructions how to use them? Basically this what is written in the PR description.
Done. |
Benchmark for 30f23e8Click to view benchmark
|
Benchmark for 80f8904Click to view benchmark
|
About switching to libtest mimic, what is your idea on how it should be done? I assume the idea is we would get rid of our test running code Upon an initial look at it, from its docs, looks like output capture and This means we lose the way to see individual test outputs when running the test suite (LukasKalbertodt/libtest-mimic#9). Another potential issue is the filtering support we have built in our test runner over time, which we would need to reimplement on top of the new system. It certainly seems appealing to offload some of this test running code, but my concern is we might not be easily able to extend in the future if we needed. What are we gaining by introducing libtest mimic? Is it just to make the output as similiar to |
We can still use what we have: I think that for compilation, we can use the For tests that need the VM to run, we can keep what we have because we do not capture output, right? For "IR generation tests", we could change them to start using "sdk harness" can also benefit from this workflow, but we would need to build the
I think we have two options. One is to keep the current filter and only return the filtered tests to A second option would be to always expand every single option, and let
It could be something like
This would work because our "test finder" can append "(abi)" when a test, tests the abi. I think we can think of something similar to other cases.
I think the biggest gain would be that our tests would be more "standard rust tests", which means they would work with the rest of the ecosystem. Maybe even things like: https://github.com/swellaby/vscode-rust-test-adapter But, for me, honestly, would be that |
dafae1d
to
e928b9c
Compare
Benchmark for 496f79cClick to view benchmark
|
Benchmark for 03bb5deClick to view benchmark
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left just some minor suggestion.
Benchmark for 061859dClick to view benchmark
|
Benchmark for e322267Click to view benchmark
|
Benchmark for 08445dfClick to view benchmark
|
d4bf38a
to
9c9c05e
Compare
Benchmark for d00b206Click to view benchmark
|
Benchmark for 5021abeClick to view benchmark
|
Benchmark for 0b582b3Click to view benchmark
|
Description
This PR fixes: #6376
The issue is that we use the first element of the array as its element type. This can be problematic as the first element is not "concrete". For example, when it is
Numeric
.Now we iterate the array elements, search for the first "good" element, and use its type as the array type. This means:
1 - if the type is concrete, we use it;
2 - if the type is
Numeric
, we keep searching for something better;This allows consistent arrays, but where the concrete types are at the end.
snapshot tests
This PR also brings an experiment. Now when an "e2e" test has a file
snapshot.toml
it will run ascargo insta
snapshot tests. These tests can be run as normal:cargo r -p test
, and in two new ways:Snapshots can be reviewed using normal "cargo insta" workflow.
The secret lies in https://github.com/LukasKalbertodt/libtest-mimic. I also want to suggest that all our tests migrate to this model, where we could just use "cargo test" normally. Apart from that, I opted to use the
forc
executable, which means tests will run in parallel. In the future, if we want to poke compiler internals, we can expose them using flags, or create a special binary to fit our needs.snapshot.toml
is still empty because I have no idea to what put there at the moment. My initial idea was to configure what ends up into snapshots (multiple runs, compiler flags, script/contract calls) etc... But for the moment, empty is enough.Checklist
Breaking*
orNew Feature
labels where relevant.