Skip to content

[Test] Loosen managed-job --tail invalid-int assertion#9970

Open
kevinmingtarja wants to merge 1 commit into
masterfrom
zhwu/jobs-logs-tail-int-assert
Open

[Test] Loosen managed-job --tail invalid-int assertion#9970
kevinmingtarja wants to merge 1 commit into
masterfrom
zhwu/jobs-logs-tail-int-assert

Conversation

@kevinmingtarja

@kevinmingtarja kevinmingtarja commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

test_managed_jobs_logs_tail verifies that sky jobs logs --tail <non-integer>
is rejected. The assertion grepped for the exact error string
(is not a valid integer), which is more brittle than the test intends — the
point is that the bad value is rejected, not the exact wording the integer
parser uses to report it.

This relaxes the grep to match the rejection by the integer parser without
pinning the exact wording, while staying specific enough to avoid matching
unrelated errors that merely contain "invalid" (e.g. a job-id error).

Test plan

  • Test-only; no product behavior change.
  • The relaxed pattern (grep -iE "invalid.*(int|num|tail)|not a valid") still
    matches the current error output, so the rejection continues to be asserted.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates a smoke test in tests/smoke_tests/test_managed_job.py to make the validation of CLI argument parser errors more flexible by using a case-insensitive regex pattern. The reviewer noted that the new pattern "invalid|not a valid" is too broad and could cause false positives, such as matching unrelated errors like "Invalid job ID". They suggested a more specific regex pattern to target integer, numeric, or tail-related validation errors.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread tests/smoke_tests/test_managed_job.py Outdated
@kevinmingtarja kevinmingtarja force-pushed the zhwu/jobs-logs-tail-int-assert branch from 2961e6d to 4ef9c08 Compare June 26, 2026 23:09
test_managed_jobs_logs_tail checks that `sky jobs logs --tail <non-int>`
is rejected, but it grepped for the exact error string
("is not a valid integer"). Match the rejection without pinning the exact
wording, which is an implementation detail of the integer parser.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kevinmingtarja kevinmingtarja force-pushed the zhwu/jobs-logs-tail-int-assert branch from 4ef9c08 to fc02bcc Compare June 26, 2026 23:11
@kevinmingtarja kevinmingtarja enabled auto-merge (squash) June 26, 2026 23:11
@kevinmingtarja kevinmingtarja changed the title [Test] Make managed-job --tail invalid-int assertion CLI-agnostic [Test] Loosen managed-job --tail invalid-int assertion Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant