Skip to content

chore: drop duplicate --max-list check and local Azure HTTP mocks - #645

Merged
IAmJSD merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-644
Oct 26, 2026
Merged

IAmJSD merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-644

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Root cause and fix

Two leftovers from platform issues whose platform parts are already done (A14-040 and A15-007).

1. Duplicate --max-list range check (ci_cd_sanity_tests/cmd/sanity/main.go)

main() called requireInt32Range("--max-list", *maxList) and then ran the same bound check again inline. This PR:

  • removes the inline copy.
  • makes the helper compare against math.MaxInt32 instead of 1<<31-1.
  • splits the check into validateInt32Range(flagName, n) error, so it can be unit-tested without os.Exit. requireInt32Range still prints and exits with code 2.

The math import is still used by the helper. The helper's message now includes the bound and the offending value, which the deleted inline copy used to print, so no diagnostic is lost. The int32(*maxList) conversion and its #nosec G115 justification are unchanged and remain valid.

2. Local copies of the shared Azure HTTP mock

providers/azure/services/{cache,cosmosdb,database,search}/client_test.go each declared their own MockHTTPClient and createMockHTTPResponse. This PR deletes those copies and switches every call site to mocks.MockHTTPClient and mocks.CreateMockHTTPResponse from providers/azure/mocks, which compute, managedredis and synapse already use.

All four files already imported that package, so no import changes were needed. The shared double does the same thing as the deleted copies: m.Called(req), nil→error, otherwise the response. The only difference is that it panics with a clearer message on a wrong type instead of a bare type-assertion panic. No Test* functions were removed.

Closes #644

Verification

All of this is local unit evidence. No cloud calls were made. The sanity binary exits on flag validation before aws.Run. Environment for every command: AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, Go 1.26.10.

  • ci_cd_sanity_tests: go build ./..., go vet ./... and go vet -tags integration ./... all pass.
  • providers/azure: go build ./..., go vet ./... and go vet -tags integration ./... all pass.
  • Unit tests, one package at a time, all ok:
    • go test -count=1 ./cmd/sanity/ (new TestValidateInt32Range: 0, -1, 1, 5, MaxInt32, MaxInt32+1)
    • go test -count=1 ./services/cache/
    • go test -count=1 ./services/cosmosdb/
    • go test -count=1 ./services/database/
    • go test -count=1 ./services/search/
  • golangci-lint v2.10.1 run --allow-parallel-runners reports 0 issues in both modules: providers/azure on ./services/{cache,cosmosdb,database,search}/ and ci_cd_sanity_tests on ./cmd/sanity/.
  • gocyclo -over 10 -ignore "_test\.go" ci_cd_sanity_tests/cmd/sanity/main.go produces no output.
  • gofmt -l on the touched files produces no output.
  • A duplicate check (grep -rn "type MockHTTPClient\|func createMockHTTPResponse" providers/) now finds only providers/azure/mocks/azure_mocks.go.

Fail-before / pass-after. This is a refactor with no behavior bug, so the honest "before" evidence is limited to the following:

  • With the new test against the old main.go, the build fails with undefined: validateInt32Range. The helper was not testable before.
  • Binary behavior is the same apart from the message. Before, for both --max-list=0 and --max-list=2147483648, it printed ERROR: --max-list must be between 1 and math.MaxInt32 and exited with rc=2. After, it prints ERROR: --max-list must be between 1 and 2147483647, got 0 (or got 2147483648) and still exits with rc=2.
  • The Azure part changes only test code. The evidence is that the four packages still pass while using the shared double.

Parent verification at 5de4e1c (branch is up to date with main): go test -race -short -count=1 ./... passes in the ci_cd_sanity_tests and providers/azure modules. This is the only branch for its modules in this batch; go#643 touches different files.

Out of scope

  • No other changes. I did not look for further duplicated test doubles beyond the four files named in the issue.

Labels

Maintainer: please mirror the issue's triage labels onto this PR: type/chore, priority/p3. The issue has no triaged label.

🤖 Generated with Claude Code

The sanity binary validated --max-list twice: once through
requireInt32Range and again inline in main(). Remove the inline copy,
have the helper compare against math.MaxInt32 instead of 1<<31-1, and
split the check into validateInt32Range so it is unit-testable. The
error message now keeps the bound and offending value the inline copy
used to print.

The cache, cosmosdb, database and search client tests declared their
own MockHTTPClient and createMockHTTPResponse. Delete those copies and
use mocks.MockHTTPClient and mocks.CreateMockHTTPResponse from
providers/azure/mocks, as compute, managedredis and synapse already do.

Closes LeanerCloud#644

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@IAmJSD IAmJSD added type/chore Maintenance / non-user-visible priority/p3 Polish / idea / may never ship labels Oct 26, 2026
@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 5de4e1c0a9f90c18e1cf3951f81044a8cf740d2f

Blocking:

  • none

Non-blocking:

  • ci_cd_sanity_tests/cmd/sanity/main_test.go:14: on a 32-bit int the aboveMax case wraps to math.MinInt32, so there it tests "negative" a second time, not "above the bound". That's harmless (CI is 64-bit, and on 32-bit the > MaxInt32 branch can't be reached anyway), and the comment already says so.
  • The shared mocks.MockHTTPClient has two fields no caller uses (ResponseBody, StatusCode). That was already true and isn't in scope here.

Attacked:

  • Checked that the Azure part is purely mechanical. Every -/+ line in providers/ is either the 4x deleted MockHTTPClient/Do/createMockHTTPResponse definitions or a 1:1 rename to mocks.MockHTTPClient{} / mocks.CreateMockHTTPResponse(, with no other edits. The func Test* counts are unchanged (cache 49, cosmosdb 52, database 62, search 48). grep -rn "type MockHTTPClient\|func createMockHTTPResponse" providers/ now finds only providers/azure/mocks/azure_mocks.go, so the issue's "neither duplicate remains" holds. The shared Do matches the deleted copies (nil -> error, otherwise the response) and only adds a clearer panic message.
  • Sanity check: built the binary and ran --max-list=0 and --max-list=2147483648. Both give rc=2 with must be between 1 and 2147483647, got N, before any AWS call. Mutation probes: dropping the upper bound fails above_upper_bound, and dropping the lower bound fails zero/negative. Both were restored. There is no behaviour bug to show fail-before for (it's a chore refactor); the probes show the new test bites.
  • Ran in both modules (ci_cd_sanity_tests, providers/azure): go build ./..., go vet ./..., go vet -tags integration ./..., go test -race -short -count=1 on ./cmd/sanity/ and on ./services/{cache,cosmosdb,database,search}/ (all ok), golangci-lint v2.10.1 with and without --build-tags integration (0 issues), and gocyclo -over 10 cmd/sanity/main.go (clean). The touched packages have no integration-tagged tests.

Body-stale:

  • none

Reviewer: rF2. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at c12870ed9d37483d5b91df2bbdbeeb9990da7017 (main merged in; the PR's diff is unchanged, so earlier reviews carry over; every module builds, vets and passes go test -race). CI Success and pre-commit green. Merged by IAmJSD's agent at IAmJSD's request (acting owner while the maintainers are away), under the bar IAmJSD set: one approving independent review (rF2) on the head. Squash-merged at exactly c12870ed9d37483d5b91df2bbdbeeb9990da7017.

@IAmJSD
IAmJSD marked this pull request as ready for review October 26, 2026 20:23
@IAmJSD
IAmJSD merged commit 811d660 into LeanerCloud:main Oct 26, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority/p3 Polish / idea / may never ship type/chore Maintenance / non-user-visible

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore: remove the duplicate sanity --max-list check and the local Azure HTTP mock copies

1 participant