test(fileservice): isolate external object storage tests

[gouhongshen]

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

Related to #26541

What this PR does / why we need it:

Root cause

The fileservice test argument helper mixed the local disk fixture with s3.json, s3_fs_test_new.xml, and all ambient TEST_S3FS_* variables. Consequently, coverage's go test -short could execute live Aliyun/QCloud tests whenever credentials were present, so an unrelated external TCP timeout failed the unit-test job.

Changes

• split local disk and external object-storage argument sources
• retain shared object-storage and S3FS contract assertions while exposing explicit external test entry points
• require both non-short mode and MO_RUN_EXTERNAL_OBJECT_STORAGE_TESTS=1 before external configuration is read or a provider can be contacted
• fail an explicitly enabled external run when its configuration is absent or malformed
• defer XML test configuration loading until the explicitly enabled external test starts

This is test-only: no production API, storage format, retry behavior, or runtime path changes.

Issue-to-test proof

• TestShouldRunExternalObjectStorageTests covers the explicit opt-in contract, disabled default, short-mode precedence, and invalid values.
• TestLocalObjectStorageArgumentsIgnoreExternalConfiguration proves a poisoned TEST_S3FS_ALIYUN value cannot enter the local tier.
• The focused object-storage run with poisoned Aliyun/QCloud specs pointed at 127.0.0.1:1 executes only the disk fixture and passes in both short and normal modes.
• All external entry points explicitly skip without the opt-in, preventing ambient credentials from reaching a provider.

Validation

• TEST_S3FS_ALIYUN=...127.0.0.1:1 .agents/skills/mo-dev/scripts/mo-cgo-test -v -count=1 -timeout=120s -short -run '^(TestShouldRunExternalObjectStorageTests|TestLocalObjectStorageArgumentsIgnoreExternalConfiguration|TestObjectStorages)$' ./pkg/fileservice
• TEST_S3FS_ALIYUN=...127.0.0.1:1 .agents/skills/mo-dev/scripts/mo-cgo-test -v -count=1 -timeout=120s -run '^(TestObjectStoragesExternal|TestS3FSFromExternalSpecs|TestNewS3FSFromSpec|TestQCloudSDK)$' ./pkg/fileservice
• TEST_S3FS_ALIYUN=...127.0.0.1:1 .agents/skills/mo-dev/scripts/mo-cgo-test -v -count=1 -timeout=120s -run '^TestObjectStorages$' ./pkg/fileservice
• CGO_CFLAGS="-I$PWD/cgo -I$PWD/thirdparties/install/include" CGO_LDFLAGS="-L$PWD/thirdparties/install/lib" GOWORK=off go vet -mod=readonly ./pkg/fileservice
• git diff --check

The full short package suite was also attempted with poisoned Aliyun/QCloud specs. It failed in the untouched local TestMinioSDK/file_service because the local MinIO process returned Access Denied; the same isolated test failed with that error.

Residual risk

Live provider integration was intentionally not run: it requires a maintained endpoint and explicit credentials. This PR makes that dependency opt-in so provider/network availability can no longer make required unit coverage flaky.

Landing condition

Do not land this PR alone. Current matrixorigin/CI coverage still provides TEST_S3FS_ALIYUN and TEST_S3FS_QCLOUD only to go test -short, so the new external tier would be skipped without a paired CI rollout. Before landing, a matrixorigin/CI change must remove provider credentials from coverage and add trusted, provider-isolated, non-short exact-head jobs with MO_RUN_EXTERNAL_OBJECT_STORAGE_TESTS=1, triggered pre-merge for fileservice/dependency paths and also scheduled/manual.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[XuPeng-SH]

Deep review of exact head 73964705b28cad1b2e5128684893b6f036cd574a.

Assessment: changes are required. The local/external split, short-mode precedence, deferred configuration loading, and fail-closed handling for an explicitly enabled run are sound. However, the rollout currently has no automated consumer for the newly gated external tier, so this removes Aliyun/QCloud coverage instead of isolating it.

Current matrixorigin/CI still exports TEST_S3FS_ALIYUN/TEST_S3FS_QCLOUD only into coverage jobs that invoke go test -short; this PR's guard necessarily skips every external entry point there. The CI repository has no object-storage integration workflow and its default branch has no reference to MO_RUN_EXTERNAL_OBJECT_STORAGE_TESTS or the new test names. The previously proposed paired changes, matrixorigin/CI#416 and #26612, were both closed unmerged. The green checks on this PR therefore establish only that local disk tests still pass and live providers are skipped.

Please land a paired CI path before or with this change: execute the exact trusted PR head without -short, set MO_RUN_EXTERNAL_OBJECT_STORAGE_TESTS=1, isolate Aliyun and QCloud credentials/jobs, and keep a scheduled/manual run plus a path-filtered pre-merge run for fileservice/dependency changes. Remove the cloud secrets from required short coverage once that replacement exists. The explicit-enable/no-config failure already gives that workflow a useful fail-closed guard.

Validation on macOS with source-matched native artifacts: the focused short and normal suites passed with Aliyun/QCloud specs poisoned to 127.0.0.1:1; focused race passed; explicit enablement with no configuration failed immediately as intended; go vet ./pkg/fileservice and git diff --check passed. Live provider tests were not run because no credentials were used.

[XuPeng-SH]

[P1] Wire this opt-in to an automated external-storage workflow before landing. On the current CI default branch, the only jobs receiving TEST_S3FS_ALIYUN/TEST_S3FS_QCLOUD run go test -short, so shouldRunExternalObjectStorageTests always returns false. There is no workflow setting this new variable or selecting the new test entry points; the earlier paired CI#416 / matrixone#26612 rollout was closed unmerged. Consequently every automated run now reports success while skipping Aliyun/QCloud, turning the original flaky coverage into no provider coverage. Please pair this with a trusted exact-head, non-short workflow that sets this flag, runs providers in isolated jobs (pre-merge for relevant paths and scheduled/manual), and removes the credentials from short coverage.

[gouhongshen]

Confirmed valid. Current matrixorigin/CI coverage-ut.yaml exports TEST_S3FS_ALIYUN and TEST_S3FS_QCLOUD only to go test -short; CI main has no object-storage integration workflow, and MatrixOne has no caller. Therefore this PR alone would remove provider coverage rather than isolate it.

The required replacement needs a paired matrixorigin/CI change that owns trusted runner/secrets: remove these credentials from coverage, add provider-isolated non-short exact-head jobs with MO_RUN_EXTERNAL_OBJECT_STORAGE_TESTS=1, and retain path-filtered plus scheduled/manual triggers. The closed CI#416 and matrixone#26612 show the intended split, but neither landed. That cross-repository CI/secret work is outside the current MatrixOne-only PR authority, so I am leaving this thread unresolved and holding this PR from landing until the paired CI path is authorized and available.

[gouhongshen]

Reconfirmed on exact head 3fa1d644eb13c142c30e863210ee688bec35c479: “the previous automated-coverage rollout blocker is still open.” Current matrixorigin/CI@main still exports TEST_S3FS_ALIYUN and TEST_S3FS_QCLOUD in coverage-ut.yaml and runs go test ... -short; it has no MO_RUN_EXTERNAL_OBJECT_STORAGE_TESTS consumer or object-storage workflow, and this MatrixOne branch has no caller. The P1 is valid and not stale.

The required fix is a coordinated matrixorigin/CI change that owns trusted runners/secrets: remove provider credentials from short coverage and add exact-head, non-short, provider-isolated opt-in jobs with relevant path, scheduled, and manual triggers. That external CI/secret administration is outside this MatrixOne-only PR authority. The PR body states the same landing condition, so I am leaving this thread unresolved and holding the PR until an authorized paired change can be linked.

[XuPeng-SH]

Deep review of exact head 3fa1d644eb13c142c30e863210ee688bec35c479.

The four-file test refactor is structurally sound: local disk fixtures cannot discover ambient cloud configuration; short mode takes precedence over the opt-in; XML loading is deferred until the external entry point; object-storage/S3FS contract bodies remain shared rather than weakened. No production path, API, storage format or runtime performance change is introduced. I inspected all changed hunks and their callers, including error/skip behavior and provider dispatch. I did not rerun live provider tests or a full CGo suite.

[P1] The previous automated-coverage rollout blocker is still open

Trigger: merge this head with the currently deployed matrixorigin/CI@main. pkg/fileservice/object_storage_arguments_test.go:80-90 requires both non-short execution and the new opt-in. The existing coverage workflow still exports Aliyun/QCloud credentials (CI .github/workflows/coverage-ut.yaml:254-255) and invokes go test ... -short (line 275). Consequently the previously executed cloud-backed contract tests now always skip. Setting the opt-in on that same short job cannot restore them.

I rechecked the current CI main workflow inventory and open CI PRs: there is no replacement object-storage integration workflow; this MatrixOne head also has no workflow/runner consumer of MO_RUN_EXTERNAL_OBJECT_STORAGE_TESTS. Therefore this is a concrete removal of automated provider coverage, not just a request for more validation evidence. The new PR-body landing condition accurately acknowledges the dependency, but prose does not implement or close it.

Please provide and land the paired automation before or atomically with this change: trusted exact-head, non-short provider-isolated runs with MO_RUN_EXTERNAL_OBJECT_STORAGE_TESTS=1; a pre-merge path for relevant fileservice/dependency changes plus scheduled/manual coverage; remove provider credentials from the short coverage job once the replacement exists. Link the actual paired change and its caller so the landing condition is verifiable. The current local/external separation can remain.

Request changes solely for the still-unresolved coverage handoff. No new independent implementation blocker was found, and no additional unrelated redesign is requested.