Skip to content

ci: run the RFC 0019 localstack S3 tests in the s3-integration job - #300

Merged
jensholdgaard merged 1 commit into
mainfrom
chore-wire-localstack-tests-ci
Jun 28, 2026
Merged

ci: run the RFC 0019 localstack S3 tests in the s3-integration job#300
jensholdgaard merged 1 commit into
mainfrom
chore-wire-localstack-tests-ci

Conversation

@jensholdgaard

@jensholdgaard jensholdgaard commented Jun 28, 2026

Copy link
Copy Markdown
Owner

What

The s3-integration (localstack) CI job's --exact list was pinned to the original four RFC 0013 tests, so the localstack tests added across the RFC 0019 Store migration (#293/#294/#297/#299) compiled under --all-targets but never actually ran in CI. This adds the six new ones to the invocation:

  • store_list_enumerates_keys_on_s3, store_list_common_prefixes_rolls_up_on_s3, store_list_with_sizes_reports_byte_lengths_on_s3, store_delete_blocking_removes_on_s3 (Store listing/delete helpers)
  • compact_partition_consolidates_on_s3_via_cas (compaction publish_cas)
  • parquet_audit_sink_persists_to_s3 (audit sink on S3)

Still --exact (not bare --ignored) so the deferred rfc0013_8_reader_forward_compat_over_store todo!() stub stays un-run.

Also folds in a one-line doc fix: WriterError::Poisoned's message still said "the .parquet.tmp is preserved on disk", stale since #294 made the writer buffer-and-put (the buffer is now discarded; nothing is put).

Why

De-risks RFC 0019 slice 3: this PR's own s3 integration (localstack) run is the first time those six localstack tests execute in CI, validating the Store-migration harness end-to-end on real AmazonS3 (localstack) before the server-level e2e tests build on it.

Tests

No new tests — wires existing #[ignore]d ones into CI. cargo fmt --check, cargo clippy -p ourios-parquet --all-targets -D warnings, YAML parse — all green locally. The six tests run in this PR's s3 integration job.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Clarified the error message shown when a parquet write fails, so it now accurately reflects that buffered data is discarded and nothing is stored.
  • Tests

    • Expanded the S3 integration test run to include additional store and compaction cases, improving coverage for ignored tests.

The s3-integration job's --exact list was pinned to the original four RFC 0013
tests, so the Store-migration localstack tests added since (querier/compactor/
audit Store listing/delete/CAS/audit-sink) compiled but never actually ran in
CI. Add the six new ones to the list (still --exact, so the deferred rfc0013_8
todo! stub stays un-run). Also: fix a stale Writer::Poisoned message that still
referenced a .parquet.tmp on disk (the writer went buffer-and-put in #294).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 730ccdb2-e117-40f9-91ae-65fccb3b6a71

📥 Commits

Reviewing files that changed from the base of the PR and between 5cd5b21 and 0246c78.

📒 Files selected for processing (2)
  • .github/workflows/ci.yml
  • crates/ourios-parquet/src/writer.rs

📝 Walkthrough

Walkthrough

The CI s3-integration job's explicit cargo test list is extended with additional Store, compaction, and audit S3 tests. The WriterError::Poisoned display message is updated to state that the in-memory buffer is discarded rather than that a .parquet.tmp file is preserved.

CI and error message updates

Layer / File(s) Summary
CI S3 test expansion and WriterError message
.github/workflows/ci.yml, crates/ourios-parquet/src/writer.rs
Adds store_list_*, store_delete_*, compact_partition_consolidates_*, and parquet_audit_sink_persists_to_s3 to the --ignored --exact test list with explanatory comments; changes Poisoned error text to indicate the in-memory buffer is discarded and nothing is written to the store.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • jensholdgaard/ourios#239: Introduced the rfc0013_* ignored S3 integration tests that are selected by the same --ignored --exact CI mechanism being extended here.
  • jensholdgaard/ourios#293: Added the RFC0013 Store S3 seam tests (store_list_*, store_delete_*) now being enrolled in CI.
  • jensholdgaard/ourios#299: Introduced parquet_audit_sink_persists_to_s3 and the "buffer discarded" WriterError::Poisoned wording that this PR adds to CI and finalizes.

Poem

🐇 A buffer once promised a file on the shelf,
But now it just vanishes, cleaning itself.
And tests in the pipeline, grown one by one,
Now audit and compact until the job's done.
No .tmp to linger, no ghost in the store—
The rabbit hops onward and tidies the floor!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the CI job and the RFC 0019 localstack S3 tests being added.
Description check ✅ Passed It explains what changed, why, and how it was tested; only the repository's exact template headings are loosely followed.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore-wire-localstack-tests-ci

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI 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.

Pull request overview

This PR ensures the s3-integration (LocalStack) CI job actually runs the newer RFC 0019 S3 integration tests that were previously compiled but skipped due to the --exact allowlist only naming the original RFC 0013 tests. It also updates WriterError::Poisoned’s display text to match the current buffer-and-put writer implementation (no temp file is left on disk).

Changes:

  • Expands the s3-integration job’s cargo test ... --ignored --exact allowlist to include six RFC 0019 Store-migration LocalStack tests.
  • Keeps the deferred rfc0013_8_reader_forward_compat_over_store todo!() stub excluded by continuing to use an explicit --exact list.
  • Fixes the WriterError::Poisoned message to reflect that the in-memory buffer is discarded and nothing is written to the store.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/ourios-parquet/src/writer.rs Updates WriterError::Poisoned’s Display message to match the buffer-and-put writer behavior.
.github/workflows/ci.yml Adds the RFC 0019 LocalStack S3 integration tests to the s3-integration job’s explicit --exact test list.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jensholdgaard
jensholdgaard merged commit 1e6c251 into main Jun 28, 2026
22 checks passed
@jensholdgaard
jensholdgaard deleted the chore-wire-localstack-tests-ci branch June 28, 2026 10:46
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.

2 participants