Conversation
The suite files live in a git submodule. Every filesystem and decoding error was discarded, so an absent submodule produced zero test cases and a passing run instead of a failure.
RFC 9651 section 4.2 combines field lines with a comma and a space. Only the comma was used, so a string split across two lines parsed to "foo,bar" instead of "foo, bar". Caught by the two_lines cases of the refreshed test suite.
Checkout skipped the test suite submodule, which is what let the silent test failure reach main. Also drop the obsolete GO111MODULE variable and stop running the whole matrix twice on pull request branches. Add Dependabot so the pinned SHAs and the submodule stay current.
Mirrors the setup used in mercure and frankenphp: a seven day cooldown before an update is proposed, then auto-merge restricted to minor and patch GitHub Actions bumps. Submodule and major updates stay manual.
RELEASE_APP_ID and RELEASE_APP_PRIVATE_KEY already live in the release environment of mercure and frankenphp, so reuse it rather than copying the private key into a second environment.
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
1 participant
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.
CI has been running zero official test-suite cases, while reporting success.
The chain
The
structured-field-testssubmodule was never checked out in CI, because the workflow had nosubmodules:key. httpwg_test.go:146 then discarded theos.Openerror, soReaddirreturned zero entries, the loop body never ran, and the test passed. Both suite tests were no-ops on every run.The bug that hid behind it
Multi-line field values were joined with
",". RFC 9651 section 4.2 joins them with", ". Inside a string the difference is visible:["\"foo", "bar\""]parsed tofoo,barinstead offoo, bar. This failedstring.json/two_lines_stringanddisplay-string.json/two_lines_display_stringas soon as the suite actually ran.TestUnmarshalItemhad the incorrect value written into it as the expectation, so it is corrected here too.Changes
test:suite loading now fails loudly on a missing submodule, an unreadable file, a decode error, or an empty case list, rather than silently running nothing.fix:join field lines with", ". Submodule refreshed fromdraft-17-70todraft-17-94, 24 commits.ci:check out submodules. Bump and pin every action to a full commit SHA. Drop the obsoleteGO111MODULE. Stop running the matrix twice per pull-request push. Addpermissions: contents: readand a concurrency group.ci:Dependabot with a 7 day cooldown, covering actions and the submodule, plus auto-merge for minor and patch action bumps, modelled on mercure and frankenphp.docs:gofmt on adictionary.gocomment.Suite cases executed: 0 in CI, 1559 locally on the old pin, 1592 now.
gofmt -lempty,go vetclean, golangci-lint v2 reports 0 issues,actionlintclean.Before merging
The auto-merge workflow needs repository configuration that is not present yet, and will fail on its first Dependabot PR until it is in place:
allow_auto_mergeis currentlyfalseon this repo; it must be enabled in Settings.dependabotenvironment. mercure usesrelease, frankenphp usesdependabot; I went with the latter.RELEASE_APP_ID(variable) andRELEASE_APP_PRIVATE_KEY(secret) are not set here. The defaultGITHUB_TOKENcannot be substituted, since it lacks theworkflowsscope and Dependabot action bumps edit.github/workflows/ci.yaml.mainhas no required status checks, so--autohas no green to wait for. Worth adding CI as required.I did not touch any repository setting. Happy to drop the auto-merge commit if you would rather land the fixes first.