Repository navigation
#50: remove test-only production functions - #67
Conversation
deyna256
left a comment
deyna256
left a comment
There was a problem hiding this comment.
Summary: the three functions are gone, deadcode is clean and the migrated tests keep their checks. One thing to fix first: a test still carries the name of the function this PR deletes.
Blocking, inline: internal/codex/stream_test.go:116.
Checked (at b2c082e):
just format --check,just lint,just deps(no file changes) andjust testpass;internal/codexoauthandinternal/storagealso pass-race -count=10 -shuffle=on.deadcode ./...reports nothing here, and exactly the three functions on the base commit.- Per-function coverage compared with the base across accesskey, codex, storage, codexoauth and execution: no function lost coverage,
ReplaceAccountCredentialsIfUnchangedgained (70.6% to 76.5%), and total statement coverage is 88.5% on both. TestCredentialReplacementFailurePreservesRecordstill fails for the reason it is named after: the SQLite trigger's constraint error, notErrConflict.- Reshaping
consumeClientStreamlikeexecution.Executor.Generate(close before readingResult, join the cleanup error) passes the same tests, so the helper's shape is not load-bearing. - No documentation references the removed functions, and constant-time comparison stays where it is used (admin token, OAuth state); access-key authentication was already a hash lookup in SQLite.
- This branch and #68 merge cleanly, and the merged tree builds, vets and passes tests.
| started := time.Now() | ||
|
|
||
| result, err := client.Generate(t.Context(), testAccount(t, "one"), request(t, `{"model":"m","input":"hi"}`)) | ||
| result, err := consumeClientStream(client, t.Context(), testAccount(t, "one"), request(t, `{"model":"m","input":"hi"}`)) |
There was a problem hiding this comment.
TestGenerateHasNoDefaultTotalTimeout at line 98 and its failure message at line 119 still name Client.Generate, which this PR deletes. #50 exists so that readers do not have to work out which path is the real one, and a test named after a removed function is exactly that cost.
Please rename both, and the ordinary result in TestStreamPreservesOrderAndAssemblesCompletedItems with them: it used to mean the non-streaming call, and now both sides of that comparison drain the same stream.
| // terminal result for tests that need to inspect a complete response. | ||
| func consumeClientStream( | ||
| client *codex.Client, | ||
| ctx context.Context, |
There was a problem hiding this comment.
Nit: ctx belongs first. context.Context documents it as the first parameter, and every function in internal/ follows that except one constructor in the gateway.
Also, this package keeps its shared test helpers in request_test.go (testAccount, testClient, request, assertJSON), and this is the only test file in internal/codex with no source file of the same name. client_test.go or request_test.go would fit the helper better than a file of its own.
|
|
||
| if verified { | ||
| t.Error("Verify() accepted a mismatched key or record") | ||
| if valid && candidate == tt.hash { |
There was a problem hiding this comment.
Nit: the digest presented as key case now passes on valid == false alone, so it no longer compares anything against a record, and TestMalformedKeysReturnNoVerificationHash already covers that input. Asserting !valid for it, or dropping the case, keeps the table honest.
| t.Fatal(err) | ||
| } | ||
| if err := store.ReplaceAccountCredentials(context.Background(), "account", account.OAuthCredentials{ | ||
| if err := store.ReplaceAccountCredentialsIfUnchanged(context.Background(), previous, account.OAuthCredentials{ |
There was a problem hiding this comment.
Nit: the conditional call has a second way to fail. A stale previous returns ErrConflict and this test would still pass, although the trigger never ran. I checked that it currently fails with the constraint error, so !errors.Is(err, storage.ErrConflict) here would keep the test pinned to the failure it is named after.
|
Addressed the review feedback, including the obsolete test names, shared test helper, and test assertions. The full test suite, targeted race tests, and GitHub quality check are passing. Ready for re-review. |
There was a problem hiding this comment.
lgtm, thanks!
Checked out a05c248: deadcode ./... reports nothing where the merge base listed exactly the three removed functions, per-function coverage loses nothing but them (ReplaceAccountCredentialsIfUnchanged gains, total 86.8% to 86.9%), and just format --check, just lint, just deps, just test plus -race -count=5 -shuffle=on on the five touched packages all pass.
Three test nits left over are mine to clean up in main after this merges, nothing for you to do here.
2 participants
Problem
Three production functions were only exercised by tests, even though the gateway uses different paths. This left unused code in the application and meant some tests were not checking the actual runtime contracts.
Changes
codex.Client.Generateand migrate its tests toClient.Streamusing a test-only stream consumer.storage.Store.ReplaceAccountCredentialsand update tests to use conditional credential replacement.accesskey.VerificationHash.Verifyand exerciseaccesskey.Hashdirectly.Validation
just format --checkpassed.just lintpassed.just testpassed with race detection and shuffled execution.deadcode ./...reported no unreachable functions; JSON output also confirmed zero results.git diff --checkpassed.Closes #50