-
Notifications
You must be signed in to change notification settings - Fork 18k
cmd/compile: short tests are not short enough #26469
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
Comments
we could maybe run this in non-short mode only. @griesemer Slow compiler tests (>.1 sec):
The first one we should fix @dr2chase . |
TestGenFlowGraph generates 766 functions, compiles that, and runs it. For each function the runner first interprets it up to 4096 steps (it depends on the function, most are 512) and if that terminates, calls the function and compares the results. One way to speed it up would be to quit testing it for GO_SSA_PHI_LOC_CUTOFF=0, because it turns out that we ignore that parameter now.
That cuts the test time in half with no loss of information.... |
Change https://golang.org/cl/125075 mentions this issue: |
The test runs far too long for -short mode (4 seconds). Also removed useless test of now-disconnected knob (GO_SSA_PHI_LOC_CUTOFF), which cuts 4 seconds to 2 seconds (which is still too long), and finished removing the disconnected knob. Updates #26469. Change-Id: I6c594227c4a5aaffee46832049bdbbf570d86e60 Reviewed-on: https://go-review.googlesource.com/125075 Run-TryBot: David Chase <[email protected]> Reviewed-by: Keith Randall <[email protected]>
The TestFormats test could be run in non-short mode only as long as we run it from time to time. It does verify that the various format strings used in compiler errors use the correct format strings, etc. |
@griesemer: we have a longtest builder which will run this test on every commit. |
Change https://golang.org/cl/125037 mentions this issue: |
Update #26469 Change-Id: Id8b8d1c0db48374d5d3dc663a77187a73f60c9a5 Reviewed-on: https://go-review.googlesource.com/125037 Run-TryBot: Keith Randall <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]> Reviewed-by: David Chase <[email protected]> TryBot-Result: Gobot Gobot <[email protected]>
Should we put a general recommendation about wether test falls under short category somewhere. Is threshold on 1 second or 0.1 seconds? |
@randall77 are you sure? Do you have windows longtest builder? Most changes to Go repo and tools are developed on non-Windows computers. So Go Team developers do not see Windows problems that their changes introduce. Alex |
Yes, we run the longtest builder on every commit. The longtest builder is linux/amd64, there's no windows one (or arm, or ...). |
I doubt anyone checks long tests on windows. I rarely do it myself, because I hardly have time for short tests. Just as an exercise, I run long test for cmd/go for current tip:
and the test did not even finish. But the short test is fine:
If you remove tests from long test, they will slowly become broken. Alex |
Change https://golang.org/cl/126995 mentions this issue: |
Change https://golang.org/cl/127055 mentions this issue: |
Change https://golang.org/cl/127095 mentions this issue: |
Change https://golang.org/cl/127115 mentions this issue: |
Change https://golang.org/cl/127116 mentions this issue: |
The approach taken by the last few changes that make basic arithmetic tests depend on the testing package look not really useful to me, except as a experiment. When the logic to run the test framework works, much of the compiler and runtime are already implicitly tested. So given those are failing, the testing package probably gives segfaults first or behaves funny before any useful test code is even run. This also makes implementing a new Go compiler step by step harder and my even make porting harder since the unit of code that needs to work before these tests now run is much bigger. All in all this approach to testing the compiler really confuses me. |
I guess it depends on what you see these tests as being useful for. I seem them mainly as regression tests, making sure that the compiler handles all the weird edge cases correctly. Sure the I can see the argument that not depending on |
Before this CL we would build&run each test file individually. Building the test takes most of the time, a significant fraction of a second. Running the tests are really fast. After this CL, we build all the tests at once, then run each individually. We only have to run the compiler&linker once (or twice, for softfloat architectures) instead of once per test. While we're here, organize these tests to fit a bit more into the standard testing framework. This is just the organizational CL that changes the testing framework and migrates 2 tests. Future tests will follow. R=go1.12 Update #26469 Change-Id: I1a1e7338c054b51f0c1c4c539d48d3d046b08b7d Reviewed-on: https://go-review.googlesource.com/126995 Run-TryBot: Keith Randall <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: David Chase <[email protected]>
R=go1.12 Update #26469 Change-Id: Iad75edfc194f8391a8ead09bfa68d446155e84ac Reviewed-on: https://go-review.googlesource.com/127055 Run-TryBot: Keith Randall <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: David Chase <[email protected]>
Update #26469 R=go1.12 Change-Id: Ib9a00ee5e98371769669bb9cad58320b66127374 Reviewed-on: https://go-review.googlesource.com/127095 Run-TryBot: Keith Randall <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: David Chase <[email protected]>
Update #26469 Change-Id: I1188e49cde1bda11506afef6b6e3f34c6ff45ea5 Reviewed-on: https://go-review.googlesource.com/127115 Run-TryBot: Keith Randall <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: David Chase <[email protected]>
Running all.bash (which does go test -short std cmd), my target is usually that individual tests should run in under 1 second.
These tests take quite a bit longer:
Can these be dialed back, either actually shortened or just have parts skipped when testing.Short() is true?
/cc @aclements @griesemer @randall77
The text was updated successfully, but these errors were encountered: