Repository navigation
Conversation
`-prod` removes assert statements, and that silently changes the behaviour of any
test that performs work inside one. In this repository that reached Windows as a
hang rather than a failure: `test_stdio_reader_processes_frame_before_eof` and
`test_integration_stdio_initialize_completion_and_hover` both did
assert os.fd_dup2(transport.read_fd, 0) >= 0
so under `-prod` the descriptor was never redirected and the following read
blocked forever. Nothing caught it because the `-prod` step was gated to Linux:
- name: Run tests with production optimizations
if: runner.os == 'Linux'
Two changes:
1. Add the same step for Windows. `V_MACOS_V3_NO_FALLBACK` is "0" to match the
existing Windows test step, since Windows defaults to V1 and 0 permits
fallback.
2. Put `timeout-minutes: 30` on both. A hang in a test step otherwise consumes the
job's entire budget and reports nothing useful; a bounded step turns it into
a red build that says which step stopped. The Linux step gets the same guard
because it has the same failure mode. Drop that one line if you would rather
keep the diff to Windows only.
The Windows `-prod` step deliberately does not set `VLS_VLANG_V_REPO`, so it does
not repeat the slow vlang/v workspace tests. Those are already covered on Windows
by the non-prod step above, which does set it. That also keeps the added CI time
to the ~1-2 minutes the suite takes locally under `-prod`, instead of the
considerably longer run the workspace tests add on top of `-prod`.
Verified on Windows with V 0137eb5:
v -no-memory-limit -nocache -prod test vls/ -> completes in 76s
**Ordering: this must land after vlang#532.** Before that fix the two stdio tests
above hang under `-prod` on Windows, so this step goes red. With the timeout in
place it fails in a bounded, legible way instead of hanging, but it is still red.
The Windows job also needs vlang#526 (the suite must compile) and vlang#528 (the job
currently dies at `Check formatting`) to be green at all.
Practical merge order: vlang#526, vlang#527, vlang#528, vlang#532, then this.
Contributor
Author
Contributor
Author
|
Closed in favour of #536: run the -prod suite on Windows. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
-prodremovesassertstatements, and that silently changes the behaviour ofany test that performs work inside one. In this repository it reached Windows as a
hang rather than a failure. Two tests did:
The
dup2there is the operation the test depends on, not a check on it. Under-prodthe statement disappears, descriptor 0 keeps pointing at the real stdin,and the read that follows blocks forever:
Nothing caught it, because the
-prodstep only ran on Linux:Fix
1. Run the same step on Windows.
"0"matches the existing Windows test step, since Windows defaults to V1 and0permits fallback.
2. Bound both
-prodsteps withtimeout-minutes: 30. A hang in a test stepotherwise consumes the job's entire budget and reports nothing useful. A bounded
step turns it into a red build naming the step that stopped. The Linux step gets
the same guard because it has the same failure mode — drop that one line if you
would rather keep the diff Windows-only.
The Windows step deliberately does not set
VLS_VLANG_V_REPO. That env varenables the slow vlang/v workspace tests, which are already covered on Windows by
the non-
-prodstep above, which does set it. Leaving it off keeps the added CItime to roughly the 1-2 minutes the suite takes locally under
-prod, instead ofthe much longer run those tests add on top of
-prod.Validation
V
0137eb5(thevlang/vrevision CI builds from), Windows:76 seconds unbounded versus never terminating before #532. The 30-minute budget
is deliberately generous headroom over that.
I could not get a trustworthy figure for
-prodcombined with the vlang/vworkspace tests on this machine: another V build was running a full
test .concurrently and the two contended for all cores, so the number I got (over 50
minutes) reflects the contention, not the workload. That is exactly why the step
does not enable those tests.
Ordering constraint — please read before merging
This must land after #532. Before
that fix the two stdio tests above hang under
-prodon Windows, so this stepgoes red. With the timeout it fails in a bounded, legible way rather than
hanging, but it is still red.
The Windows job also needs #526 (the
suite must compile) and #528 (the job
currently dies at
Check formatting) before it can be green at all.Practical order: #526, #527, #528, #532, then this.
Related
#533 closes the matching codegen gap —
V3 was never compiled on Windows either — and has no ordering dependency.