Expand smoke suite to full command coverage#287
Merged
Conversation
Add ensure_card, ensure_column, ensure_comment, ensure_upload, and ensure_questionnaire discovery helpers. Stash and restore BASECAMP_TOKEN, BASECAMP_ACCOUNT_ID, and BASECAMP_PROJECT_ID in setup_extra() so env vars survive test_helper.bash's setup() unset. Fix ensure_schedule to use `schedule info` instead of nonexistent `schedule show`.
Add schedule info/entries, docs show, messages show, todos show, people pingable, auth token, config project, version command, and out-of-scope entries for login/logout/setup. Replace plain skip and return 0 with mark_unverifiable so every test path writes a trace. Thread --schedule into misc_read schedule tests.
Todos: create, complete/uncomplete, position, trash/restore. Cards: create (shortcut+direct), show, columns, move, step CRUD, trash/restore — all threaded with --card-table for dock safety. Comments: show and update after create. Messages: message shortcut, messages trash. Files: attach, upload shortcut, uploads create/show, files show, docs documents create.
New read-only test files: communication (messagetypes, forwards, subscriptions, boosts, comments), checkins (questions, answers), schedule (info, entries), reports (profile show), todolistgroups (show), tools (out-of-scope markers). All dock-ambiguous commands thread explicit IDs (--questionnaire, --schedule, --inbox).
New mutation test files: campfire (list, messages, post, line with --campfire threading), assign/unassign, lineup CRUD lifecycle, communication writes (subscriptions, boosts, react), misc writes (schedule settings, recordings trash/restore), webhooks CRUD.
Projects create → update → delete runs serial in Level 2 to avoid racing with project-discovering files in Level 0/1.
Add new Level 1 files (communication_write, misc_write) to orchestrator. Move projects_write from Level 1 to Level 2 serial. Fix qa-critic step 2 to parse BATS TAP for pass/fail (traces only record gaps), and step 3 to derive coverage from test inventory rather than traces.
github-actions
bot
added
tests
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 30 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="e2e/smoke/smoke_helper.bash">
<violation number="1" location="e2e/smoke/smoke_helper.bash:276">
P2: `ensure_person` is missing an `ensure_token` call. Every other `ensure_*` function guards the token (directly or via `ensure_project`) before invoking the CLI. Without it, a missing token surfaces as the misleading "Cannot list people" unverifiable marker instead of the clear "BASECAMP_TOKEN required" error.</violation>
</file>
<file name="e2e/smoke/smoke_communication_write.bats">
<violation number="1" location="e2e/smoke/smoke_communication_write.bats:55">
P2: `boost delete` is missing `-p "$QA_PROJECT"`, unlike every other command in this file. This breaks the PR's stated goal of threading explicit project IDs into all commands to prevent nondeterminism on multi-dock projects.</violation>
</file>
<file name="e2e/smoke/smoke_projects.bats">
<violation number="1" location="e2e/smoke/smoke_projects.bats:49">
P2: Use `ensure_account` (or check `BASECAMP_ACCOUNT_ID`) instead of calling `accounts list` directly. The helper already exists and prefers the env var when set, avoiding a CLI call that could mask regressions in the command being tested.
(Based on your team's feedback about preferring BASECAMP_ACCOUNT_ID and BASECAMP_PROJECT_ID environment variables when set.) [FEEDBACK_USED]</violation>
</file>
<file name="e2e/smoke/smoke_tools.bats">
<violation number="1" location="e2e/smoke/smoke_tools.bats:13">
P2: After `ensure_messageboard` fails it has already called `mark_unverifiable`/`skip` internally, so the `||` branch here (a) writes a duplicate trace with a less-specific reason, and (b) does not stop execution — the rest of the test still runs with an empty `$QA_MESSAGEBOARD`. Use `|| return 0` to bail out immediately (the skip flag is already set).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Expands the e2e/smoke BATS smoke suite to cover previously untested CLI command groups and deepen CRUD coverage, while improving smoke infrastructure (resource discovery helpers, trace accounting, and orchestration).
Changes:
- Add new smoke test files for additional command groups (e.g., webhooks, schedule, reports, tools, communication, checkins, etc.).
- Deepen existing smoke tests with more read/write operations and CRUD lifecycles across commands (cards, todos, messages, files, comments, projects).
- Update smoke harness: restore env vars in
setup_extra(), expand orchestrator test inventory, and add a QA critic skill doc for grading traces/coverage.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| e2e/smoke/smoke_webhooks.bats | Adds Level 1 webhook CRUD smoke coverage. |
| e2e/smoke/smoke_tools.bats | Adds tools (dock items) “show” coverage. |
| e2e/smoke/smoke_todos_write.bats | Expands todo write coverage (direct-verb lifecycle + todolist create). |
| e2e/smoke/smoke_todos_read.bats | Adds todos/todolists “show” coverage. |
| e2e/smoke/smoke_todolistgroups.bats | Adds todolist group list/show coverage. |
| e2e/smoke/smoke_schedule.bats | Adds schedule info/entries coverage. |
| e2e/smoke/smoke_reports.bats | Adds reports/timeline/timesheet/events/show/url/profile coverage. |
| e2e/smoke/smoke_projects_write.bats | Adds Level 2 serial project create/update/delete coverage. |
| e2e/smoke/smoke_projects.bats | Adjusts read-only semantics + adds accounts use coverage. |
| e2e/smoke/smoke_misc_write.bats | Adds schedule settings + recordings trash/restore coverage. |
| e2e/smoke/smoke_misc_read.bats | Broadens misc read-only coverage (schedule, search metadata). |
| e2e/smoke/smoke_messages_write.bats | Expands message write coverage (shortcut, update, pin/unpin, trash). |
| e2e/smoke/smoke_messages_read.bats | Adds messages “show” coverage. |
| e2e/smoke/smoke_lineup.bats | Adds lineup CRUD coverage. |
| e2e/smoke/smoke_lifecycle.bats | Expands out-of-scope lifecycle command accounting. |
| e2e/smoke/smoke_helper.bash | Adds new ensure_* helpers + restores BASECAMP_* env vars in setup hook. |
| e2e/smoke/smoke_files_write.bats | Expands file/upload/doc creation + show coverage. |
| e2e/smoke/smoke_files_read.bats | Adds vault/doc “show” coverage. |
| e2e/smoke/smoke_core.bats | Expands core command coverage (version, config, commands, help). |
| e2e/smoke/smoke_communication_write.bats | Adds subscription/boost/react mutation coverage. |
| e2e/smoke/smoke_communication.bats | Adds read-only communication coverage (types/inbox/forwards/subscriptions/boost/comments). |
| e2e/smoke/smoke_comments.bats | Adds comments show/update coverage. |
| e2e/smoke/smoke_checkins.bats | Adds checkins questions/question/answers coverage. |
| e2e/smoke/smoke_cards_write.bats | Expands cards write coverage (direct+shortcut, columns/move, steps, trash/restore). |
| e2e/smoke/smoke_cards_read.bats | Adds explicit --card-table usage + columns/show coverage. |
| e2e/smoke/smoke_campfire.bats | Adds campfire list/messages/post/line coverage. |
| e2e/smoke/smoke_assign.bats | Adds assign/unassign coverage. |
| e2e/smoke/smoke_account.bats | Adds more account-scoped read coverage (people/templates/auth token). |
| e2e/smoke/run_smoke.sh | Expands smoke orchestrator inventory across Level 0/1/2 buckets. |
| .claude/skills/qa-critic.md | Adds QA critic skill documentation for grading smoke traces and coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 374e97fe96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Add ensure_token guard to ensure_person (cubic) - Add -p "$QA_PROJECT" to boost delete (cubic) - Use ensure_account helper in accounts use test (cubic) - Fix ensure_messageboard bailout to avoid duplicate traces (cubic) - Remove duplicate schedule tests from smoke_misc_read (copilot) - Use non-routable domain (smoke-test.invalid) for webhook URLs (copilot) - Fix --campfire to --chat flag on campfire subcommands (codex) - Guard profile show for unconfigured profile environment (codex)
Contributor
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="e2e/smoke/smoke_tools.bats">
<violation number="1" location="e2e/smoke/smoke_tools.bats:13">
P2: This reverts `mark_unverifiable` back to a bare `return 0`, which contradicts the PR's stated goal and breaks consistency with every other `ensure_*` call site in the smoke suite. The bare `return 0` silently passes the test without recording a trace entry at the call site, which undermines the "complete trace accounting" this PR aims for.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Profile-based auth: stash/restore BASECAMP_PROFILE and BASECAMP_LAUNCHPAD_URL through test_helper's temp HOME, copy config dir into temp HOME. Dock tool safety: filter ensure_* dock lookups on .enabled == true so disabled tools (vault, chat, inbox, questionnaire) are properly detected as unavailable. Test fixes found by live run: - config project needs explicit -p flag (non-interactive) - webhooks create needs --types flag (SDK validates) - todolists show: guard SDK deserialization bug (TodolistOrGroup0 union mismatch) - lineup create: use explicit date format, guard for missing API - search metadata / reports schedule / timesheets: guard for env-specific features - messagetypes list: guard for environments without message types - files_read/messages_read/files_write: add ensure_vault/ensure_messageboard to setup_file Orchestrator: propagate BASECAMP_LAUNCHPAD_URL, strip allowlist comments before matching. QA allowlist: seed with environment-specific gaps and known SDK issues.
Contributor
There was a problem hiding this comment.
4 issues found across 14 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="e2e/smoke/.qa-allowlist">
<violation number="1" location="e2e/smoke/.qa-allowlist:8">
P2: Every new allowlist entry omits the `— <owner>` required by the format documented in the file header (`<test name> # <reason> — <owner>`). Without owners, it's unclear who is responsible for closing each gap when entries expire at the next release cycle.</violation>
</file>
<file name="e2e/smoke/smoke_files_read.bats">
<violation number="1" location="e2e/smoke/smoke_files_read.bats:9">
P2: Adding `ensure_vault` to `setup_file` will skip all 6 tests in the file if no vault is found, but only the "vaults show" test actually needs `QA_VAULT`. The other five tests (files list, vaults list, docs list, uploads list, docs show) don't depend on it and would be needlessly skipped. Keep vault discovery in the individual test that requires it.</violation>
</file>
<file name="e2e/smoke/smoke_core.bats">
<violation number="1" location="e2e/smoke/smoke_core.bats:42">
P2: `ensure_project` already writes a `mark_unverifiable` trace internally before returning 1. Calling `mark_unverifiable` again at the call site produces duplicate trace entries. Use `|| return 0` to exit the test cleanly after the helper's own trace.
(Based on your team's feedback about avoiding duplicate `mark_unverifiable` at `ensure_*` call sites.) [FEEDBACK_USED]</violation>
</file>
<file name="e2e/smoke/smoke_misc_read.bats">
<violation number="1" location="e2e/smoke/smoke_misc_read.bats:32">
P2: Missing `return 0` after `mark_unverifiable` — execution falls through to `assert_json_value` which will fail when the command returned a non-zero status.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Expands the e2e/smoke suite to exercise previously untested command groups and deepen coverage with CRUD-style flows, while improving smoke orchestration/auth handling and QA trace accounting.
Changes:
- Added many new smoke BATS files to cover additional command groups (webhooks, tools, reports, schedule, communication, checkins, etc.).
- Extended existing smoke tests with deeper “read/write + lifecycle” coverage (cards, todos, messages, comments, files).
- Enhanced smoke harness/orchestrator: profile-based auth support, env restoration, new allowlist entries, and QA-critic guidance.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| e2e/smoke/smoke_webhooks.bats | Adds webhook CRUD smoke coverage using .invalid endpoints. |
| e2e/smoke/smoke_tools.bats | Adds basic tools show smoke coverage. |
| e2e/smoke/smoke_todos_write.bats | Adds direct-verb todo CRUD-ish mutations (complete/uncomplete/position/trash/restore). |
| e2e/smoke/smoke_todos_read.bats | Adds todos show and todolists show read coverage (+ SDK-bug handling). |
| e2e/smoke/smoke_todolistgroups.bats | Adds todolist group list/show coverage. |
| e2e/smoke/smoke_schedule.bats | Adds schedule info/entries coverage using explicit schedule IDs. |
| e2e/smoke/smoke_reports.bats | Adds coverage for reports, timeline, timesheet, events, url parse, profile. |
| e2e/smoke/smoke_projects_write.bats | Adds Level 2 serial project create/update/delete lifecycle. |
| e2e/smoke/smoke_projects.bats | Clarifies read-only scope; adds accounts use smoke test. |
| e2e/smoke/smoke_misc_write.bats | Adds schedule settings mutation + recordings trash/restore flow. |
| e2e/smoke/smoke_misc_read.bats | Re-scopes to read-only misc; adds search metadata coverage. |
| e2e/smoke/smoke_messages_write.bats | Deepens message create/update/pin/unpin/trash + shortcut coverage. |
| e2e/smoke/smoke_messages_read.bats | Adds message discovery + messages show; adjusts setup. |
| e2e/smoke/smoke_lineup.bats | Adds lineup smoke coverage with environment-dependent unverifiable paths. |
| e2e/smoke/smoke_lifecycle.bats | Expands out-of-scope coverage inventory for interactive/infra commands. |
| e2e/smoke/smoke_helper.bash | Adds profile-based auth handling + more env restoration; new ensure_* helpers. |
| e2e/smoke/smoke_files_write.bats | Adds upload/attach/uploads/doc create coverage and folder create test. |
| e2e/smoke/smoke_files_read.bats | Adds vault show + docs show coverage; adjusts setup. |
| e2e/smoke/smoke_core.bats | Adds version/help/commands and config init/set/unset/project coverage. |
| e2e/smoke/smoke_communication_write.bats | Adds subscriptions/boost/react mutation coverage. |
| e2e/smoke/smoke_communication.bats | Adds read-only comms coverage (messagetypes, forwards, subs, boosts, comments). |
| e2e/smoke/smoke_comments.bats | Adds comments show/update tests chained from created comment. |
| e2e/smoke/smoke_checkins.bats | Adds checkins questions/question show/answers coverage. |
| e2e/smoke/smoke_cards_write.bats | Adds comprehensive card CRUD-ish flow (create/show/columns/move/steps/trash/restore). |
| e2e/smoke/smoke_cards_read.bats | Adds card list/columns/show read-only coverage with explicit card-table ID. |
| e2e/smoke/smoke_campfire.bats | Adds campfire list/messages/post/line coverage using explicit chat ID. |
| e2e/smoke/smoke_assign.bats | Adds assign/unassign flows with a freshly created todo. |
| e2e/smoke/smoke_account.bats | Adds people show/pingable, templates show, auth token coverage. |
| e2e/smoke/run_smoke.sh | Updates orchestrator: profile/token auth, parallelism detection, expanded suite lists, allowlist matching. |
| e2e/smoke/.qa-allowlist | Adds allowlist entries for known environment-dependent gaps. |
| .claude/skills/qa-critic.md | Adds QA critic skill guide for grading smoke runs via TAP + trace files. |
Comments suppressed due to low confidence (2)
e2e/smoke/smoke_files_write.bats:14
- The test name says it “creates a vault”, but
basecamp files folders createcreates a folder (vault is the overall tool). Renaming the test to reflect the actual operation will keep the smoke inventory/coverage reporting clearer.
@test "files folders create creates a vault" {
run_smoke basecamp files folders create "Smoke vault $(date +%s)" -p "$QA_PROJECT" --json
assert_success
e2e/smoke/run_smoke.sh:10
- The file header says Level 1+ runs serially, but the script actually runs Level 1 with
bats -j(parallel). Please update the header comment to match the current behavior (or change the invocation) so the usage docs don’t mislead readers.
# Runs Level 0 (read-only) tests in parallel, then Level 1+ serially.
# Pass/fail is determined by bats exit codes.
# Traces (QA_TRACE_DIR) record coverage gaps (unverifiable, out-of-scope).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add owner tags to all allowlist entries - Remove ensure_vault/ensure_messageboard from setup_file where the command itself IS the test (vaults list, messageboards show) - Fix duplicate mark_unverifiable traces (use || return 0 since ensure_* traces internally) - Add return 0 after mark_unverifiable in smoke_misc_read - Respect XDG_CONFIG_HOME in smoke_helper config dir - Filter blank lines from allowlist matching in run_smoke.sh - Update qa-critic prerequisites to mention BASECAMP_PROFILE - Add clarifying comments on ensure_* || return 0 pattern
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
make checkpasses (CI does not run smoke tests)BASECAMP_PROFILE=dev make smokeagainst live BC3 dev (149 tests, 0 failures, 23 allowlisted unverifiable)Findings from live run
todolists showreturns empty data —TodolistOrGroup0union type expects{"todolist":{...}}but API returns flat JSON. Tracked as allowlisted gap.