Skip to content

ci: run the test suite on push and pull request - #300

Open
userFRM wants to merge 1 commit into
deepentropy:mainfrom
userFRM:feat/ci-test-workflow
Open

ci: run the test suite on push and pull request#300
userFRM wants to merge 1 commit into
deepentropy:mainfrom
userFRM:feat/ci-test-workflow

Conversation

@userFRM

@userFRM userFRM commented Jul 29, 2026

Copy link
Copy Markdown

Summary

  • There is no test workflow. .github/workflows holds only the two documentation jobs, so nothing runs the 802 unit tests on a push or a pull request.
  • Adding one first needs the test targets to compile. tests/ib_paper_compat and tests/scenarios were left behind by three struct changes — ControlCommand::FetchContractDetails gained filters, and OrderRequest's trailing-stop and adjustable-stop variants gained trail_stop_price and the adjustable trailing pair. Thirteen literals, all mechanical.
  • The workflow installs tzdata first. config::expiry_tests resolves zones by their legacy aliases (US/Eastern), which live in tzdata's backward-compatibility file and are absent from minimal installs — those two tests fail on such a box for that reason alone, which is why they look like permanent failures locally.

Closes #301.

What this deliberately does not run, and why

  • The integration tests. The workflow builds --all-targets, so they must keep compiling, but it runs only --lib. With the compile fixed, nine of them fail: a stale coverage manifest (ControlCommand::Ping has no mapping), three PnL scenarios, and several reconnect tests. A workflow that is red from its first run gets ignored rather than fixed, so those are worth a separate look before they gate anything.
  • The Python bindings. src/python sits behind a non-default feature, so a plain cargo test never compiles it — every Python path is currently unexercised by any automated run. Its suite has 276 passing and 23 failing, the failures all RuntimeError: Not connected, so it needs a session or those tests need skipping before it can gate merges.

Both are in ibx#300.

Test plan

  • cargo build --lib --all-targets — clean; every test target compiles for the first time.
  • cargo test --lib — 802 pass. The two config::expiry_tests failures are the tzdata aliases above and pass on a runner with the full database, which is why the workflow installs it.
  • cargo test --no-fail-fast across every target, to establish what the integration suite actually does once it compiles: 911 passed, 11 failed, 8 ignored.

🤖 Generated with Claude Code

The repository has no test workflow — `.github/workflows` holds only the two documentation jobs — so nothing has been running the unit tests on a push or a pull request.

Adding one first needs the test targets to compile. `tests/ib_paper_compat` was left behind by three struct changes: `ControlCommand::FetchContractDetails` gained `filters`, and `OrderRequest`'s trailing-stop and adjustable-stop variants gained `trail_stop_price` and the adjustable trailing pair. Thirteen literals, all mechanical.

The workflow installs `tzdata-legacy` alongside `tzdata`. `config::expiry_tests` resolves zones by their legacy aliases, and on 24.04 — which `ubuntu-latest` resolves to — those aliases are not in `tzdata` at all: they were split into a separate package, so a full `tzdata` install still leaves `/usr/share/zoneinfo/US/` absent and both tests failing. Installing only `tzdata`, as this first did, would have made the workflow red on its own first run.

Scoped to `--lib` deliberately. Building `--all-targets` keeps the integration tests honest about compiling, but they are not run: with the compile fixed, nine of them fail, and a workflow that is red from its first run gets ignored rather than fixed.

The Python bindings are built but not tested. They sit behind a non-default feature, so nothing else here compiles them, and 23 of their tests need a live session. Keeping them compiling is most of the value and costs one step.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@userFRM
userFRM force-pushed the feat/ci-test-workflow branch from d2e8f5f to fde8fce Compare July 30, 2026 07:41
@userFRM

userFRM commented Jul 30, 2026

Copy link
Copy Markdown
Author

Reviewed, and the finding is one the commit argues against itself: this workflow was red on its own first run. Fixed in fde8fce.

The message says a workflow red from the start gets ignored rather than fixed, and then installed tzdata to satisfy config::expiry_tests. On 24.04 — which ubuntu-latest resolves to — the legacy aliases those tests use are not in tzdata at all. They were split into tzdata-legacy. Verified on a 24.04 box here: tzdata 2026b installed, dpkg -L tzdata lists no zoneinfo/US path, /usr/share/zoneinfo/US/ does not exist, and exactly those two tests fail. The step's comment blamed "minimal installs"; they are absent from full ones.

Now installs both. Note that #336 fixes the same failures in the library, by resolving the legacy names when the system does not carry them — the two are complementary, and CI should be green whichever lands first.

Also added: a build of the Python bindings. They sit behind a non-default feature, so nothing else in the workflow compiles them, and nothing was keeping them compiling. Their tests need a live session and stay out, but the build is one step and most of the value.

Corrected in the message: it claimed tests/scenarios was left behind by the struct changes. No hunk touches it and it compiles unmodified — all thirteen literals are in tests/ib_paper_compat.

Not taken: pinning the actions to SHAs. It is the safer practice, but the repository's other two workflows both float on tags, and quietly diverging in a PR about running tests is the wrong place to change that policy. Worth doing across all three deliberately if wanted.

The rest checks out: failures do fail the job, no continue-on-error or || true masking anything, --all-targets builds so the integration tests stay honest about compiling, and the added struct fields use the documented not-set sentinels. Both workflow steps run clean locally.

@userFRM

userFRM commented Jul 30, 2026

Copy link
Copy Markdown
Author

No further changes needed here. The compile restoration and the CI claims both check out.

One caveat it raised, which I am leaving as designed: push only covers main, so a branch pushed without a pull request runs nothing. That is deliberate — pull requests are where the gate matters, and building every branch push costs runner time for no extra signal. Worth revisiting only if branches routinely sit unpushed to a PR for long stretches.

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.

tests: the integration targets did not compile, and nine fail once they do; the Python suite has 23 failures and is never compiled

1 participant