Skip to content

test: collapse schema enumeration tests and remove duplicates#1087

Open
tejaskash wants to merge 2 commits intomainfrom
worktree-test-cleanup-day3-4
Open

test: collapse schema enumeration tests and remove duplicates#1087
tejaskash wants to merge 2 commits intomainfrom
worktree-test-cleanup-day3-4

Conversation

@tejaskash
Copy link
Copy Markdown
Contributor

Summary

  • Collapse it.each per-value enumeration patterns into consolidated tests across 9 schema test files (constants, agent-env, agentcore-project, aws-targets, deployed-state, mcp-defs, mcp, evaluator, memory)
  • Remove deploy/dev --help unit tests already covered by integ-tests/help.test.ts
  • Remove 5 it.todo placeholders from credential-ops.test.ts

Schema tests: ~506 → ~415. All cross-field validation (superRefine), boundary, and discriminated union tests preserved.

Test plan

  • npm test passes — schema subset verified locally (415 tests), full suite should pass in CI
  • No coverage regression for src/schema/ (tests collapsed, not deleted — same code paths covered)

@tejaskash tejaskash requested a review from a team May 1, 2026 18:12
@github-actions github-actions Bot added size/l PR size: L agentcore-harness-reviewing AgentCore Harness review in progress labels May 1, 2026
@agentcore-cli-automation
Copy link
Copy Markdown

The PR description says these deploy --help / dev --help unit tests are "already covered by integ-tests/help.test.ts", but that isn't quite accurate — integ-tests/help.test.ts is only a smoke test (it checks exit code 0 and the presence of the string "Usage:"). It does not assert on specific flags or options.

Concretely, after this PR nothing verifies:

  • deploy --help lists --verbose, --yes, --json, --dry-run
  • deploy --help's --verbose description mentions "resource-level"
  • dev --help lists [prompt], --port, --runtime, --stream, --logs, and the default port 8080
  • dev --help does not show --invoke (see separate comment — this is the only regression guard against reintroducing the removed flag)

A few options:

  1. Keep the unit tests as-is and update the PR description to clarify they aren't redundant.
  2. Move the flag-presence assertions into integ-tests/help.test.ts (e.g., a table of expected substrings per command) before deleting the unit versions.
  3. Drop only the assertions that are genuinely duplicated by the integ smoke test (i.e., none — exit code and "Usage:" aren't what the unit tests were asserting), and keep the flag-level assertions.

@agentcore-cli-automation
Copy link
Copy Markdown

Removing the it('does not show --invoke flag', ...) test in dev.test.ts drops the only regression guard against reintroducing the removed --invoke flag. I grepped the repo and this assertion appears nowhere else:

$ grep -rn "\-\-invoke" src --include='*.ts'
src/cli/commands/dev/__tests__/dev.test.ts:18:    it('does not show --invoke flag', async () => {
src/cli/commands/dev/__tests__/dev.test.ts:22:      expect(result.stdout.includes('--invoke'), 'Should not show removed --invoke option').toBeFalsy();

Since this is a negative assertion (a flag was removed and shouldn't come back), it's exactly the kind of thing a smoke test can't replace. Please either keep this test or relocate the assertion into integ-tests/help.test.ts.

@agentcore-cli-automation
Copy link
Copy Markdown

The PR description asserts "No coverage regression for src/schema/ (tests collapsed, not deleted — same code paths covered)". That holds for most of the collapsed cases — zod's enum parser is the same code path regardless of which enum value you feed it — but there are two places where the collapse actually narrows coverage in a way I don't think is intended:

aws-targets.test.tsAgentCoreRegionSchema: Previously enumerated all 16 supported regions; now only asserts on us-east-1, eu-west-1, us-gov-west-1. If someone accidentally drops ap-northeast-1, ca-central-1, sa-east-1, etc. from the schema's region list, nothing here will fail. The explicit enumeration of supported regions is itself the coverage signal.

mcp.test.tsGatewayTargetTypeSchema: Previously enumerated all 6 types (lambda, mcpServer, openApiSchema, smithyModel, apiGateway, lambdaFunctionArn); now only asserts on lambda and lambdaFunctionArn. Dropping mcpServer/openApiSchema/smithyModel/apiGateway from the enum would not fail this test.

Options:

  1. Keep it.each(...) for these two (it's the one pattern where enumeration genuinely catches regressions beyond "zod enum parsing works").
  2. Replace with a single assertion against Schema.options (e.g., expect(AgentCoreRegionSchema.options).toEqual([...])) — similar to what the PR already does for MemoryStrategyTypeSchema.

FWIW, the other collapsed enums (SDKFrameworkSchema, ModelProviderSchema) are fine because matchEnumValue tests above already exercise the other values via their lowercase variants.

@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label May 1, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 1, 2026

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 42.89% 8936 / 20830
🔵 Statements 42.17% 9485 / 22491
🔵 Functions 39.66% 1537 / 3875
🔵 Branches 39.82% 5769 / 14486
Generated in workflow #2335 for commit 5c66797 by the Vitest Coverage Report Action

@tejaskash
Copy link
Copy Markdown
Contributor Author

Good catch — restored the deploy/dev help flag assertions and the --invoke negative regression guard. The integ smoke test is indeed only exit-code + "Usage:".

@github-actions github-actions Bot added size/l PR size: L and removed size/l PR size: L labels May 1, 2026
@tejaskash
Copy link
Copy Markdown
Contributor Author

Addressed: replaced the representative-sample tests with .options assertions for both AgentCoreRegionSchema and GatewayTargetTypeSchema. This catches accidental removal of any enum value without per-value it.each tests.

@tejaskash tejaskash force-pushed the worktree-test-cleanup-day3-4 branch from 2e7ac63 to 1ec6c4d Compare May 1, 2026 21:13
@github-actions github-actions Bot added size/l PR size: L and removed size/l PR size: L labels May 1, 2026
tejaskash added 2 commits May 1, 2026 18:25
…t tests

Collapse it.each per-value enumeration patterns into consolidated tests
across 9 schema test files. Each enum value was tested individually
(e.g., 4 separate 'accepts SEMANTIC', 'accepts SUMMARIZATION' tests)
-- now represented by 1-2 tests with representative values.

Remove deploy/dev --help unit tests (covered by integ-tests/help.test.ts)
and 5 it.todo placeholders from credential-ops.test.ts.

Schema tests: ~506 -> ~415. All cross-field validation, superRefine,
boundary, and discriminated union tests preserved.
…sertions

- Restore deploy --help and dev --help flag-level assertions (integ smoke
  test only checks exit code and "Usage:", not specific flags)
- Restore --invoke negative regression guard in dev.test.ts
- Replace representative-sample enum tests with .options assertions for
  AgentCoreRegionSchema and GatewayTargetTypeSchema (catches accidental
  removal of enum values)
@tejaskash tejaskash force-pushed the worktree-test-cleanup-day3-4 branch from 1ec6c4d to 5c66797 Compare May 1, 2026 22:25
@github-actions github-actions Bot added size/l PR size: L and removed size/l PR size: L labels May 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/l PR size: L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants