Forward-port the double-filename fix and test overhaul; per-run test isolation; CI split#42
Merged
Conversation
Waffle resolves the version filename (Versioning.resolve_file_name/3) and writes it into file.file_name before calling the storage adapter's put/3, so put must join the storage dir with file.file_name as-is. Re-resolving here (the pre-PR-#1 behavior) doubled prefix-style filename/2 results, storing e.g. 1_original_1_original_photo.png (#25). Unlike PR #1, path_for/3 keeps its resolving semantics (now via the fullname/3 wrapper): delete/3 and the URL builders receive the *original* client filename from Waffle, so they must resolve it exactly once. Only put receives a pre-resolved name. Note for direct callers of CloudStorage.put/3 (not via definition.store): filename/2 resolution is no longer applied inside put; pass the final object name in file.file_name.
A fresh image.png plus variants with a space, a plus sign, and an invalid (non-image) file, referenced by the incoming integration suite.
Introduce Waffle.GCSCase, an ExUnit.CaseTemplate that centralizes the integration-test setup, helpers (env_bucket/bucket_url/with_env), and assertion macros (assert_public/assert_private/assert_header/ assert_public_with_extension/assert_acls_public_reader/ delete_and_assert_gone) that were previously copy-pasted across test files. It also applies the :integration and :tmp_dir moduletags. Add feature_definitions.ex: focused, single-purpose Waffle definitions (GCSTest.*), each exercising exactly one feature, plus GCSTest.FilenameProbe (@async false + a send/self probe) used by the double-filename regression test.
gcs_features_test.exs is the canonical public-API integration suite: each test drives one Waffle feature through definition.store/url/delete against real GCS, using the single-purpose GCSTest.* definitions and the shared Waffle.GCSCase. It folds back the coverage the earlier ad-hoc suites had dropped: object ACL assertions, stored-name equality checks, the pre-store URL assertion for a definition-level bucket, and per-version extension assertions. double_filename_regression_test.exs is an explicit guard for issue #25: GCSTest.FilenameProbe announces every filename/2 invocation, so the test asserts (assert_received/refute_received) that the name is resolved exactly once per version during store and that the stored object name is not double-prefixed.
cloud_storage_test.exs now covers only the CloudStorage module boundary: pure-unit path/name construction (storage_dir/3, path_for/3, bucket/1 with a literal) that needs no network, plus the put/delete/url round-trips tagged :integration. The duplicated 'from upstream storage tests' block, the copy-pasted assertion macros, and the leftover IO.inspect calls are gone (macros now live only in Waffle.GCSCase). Delete test/support/definitions.ex: the old multi-purpose fixtures were only used by the now-removed redundant suites. Repoint cleanup.ex's after_suite teardown at GCSTest.PublicUpload.
Add a tag-taxonomy comment block to test_helper.exs explaining :integration, :tmp_dir, :bucket_with_file_and_scope, double_filename_regression, skip, and upstream_mismatch, including how each is toggled with --include/--exclude and a note that future-behavior skips should become exclude tags for 0.3. Add a 'mix test.unit' alias (test --exclude integration) with a preferred_env so the fast, offline unit tests run without GCS credentials, and document the env vars and example invocations in the README.
Document how to bring the double-filename fix and the test overhaul forward into main (0.3): the precise put/3 + path_for/3 + fullname/3 lib delta and why it differs from PR #1, the orthogonal token-fetcher divergence to avoid dragging along, the cherry-pick order for the four test commits with their conflict points, and the version-gating flip that makes the :bucket_with_file_and_scope test run (and fail) once main hits 0.3. Lives under plans/, which is outside the mix.exs files: whitelist and so is excluded from the published hex package.
…tooling Distilled from a full review of the lib and test suite. Index in plans/README.md; work items tracked on GitHub as milestone v0.3 (issues #27-#37) plus #38 (offline suite), #39 (client rewrite), and #40 (migration/repair tooling). forward-port-to-main.md updated in place: the section 4 version-gating question is decided as option (a) (bucket-from-scope, #33), and WAFFLE_BUCKET2 is now available as a GHA repository secret.
- forward-port guide: the feature-suite fixture images live outside the four test commits and must be pulled across separately. - Correct the CI finding: main's workflow already triggers on main with a modern matrix; issue #36 narrows to fork-PR secrets, WAFFLE_BUCKET2 passthrough, and the shared-bucket matrix hazard. - Record the working priority within v0.3: #35, #36, #37 first.
…-friendly Closes #35. The after-suite cleanup previously deleted every object in the bucket and ran unconditionally. Three consequences: pointing WAFFLE_BUCKET at a bucket containing real data destroyed it; concurrent runs (e.g. a CI matrix) sharing one bucket wiped each other's in-flight objects; and credential-less runs (mix test.unit) crashed in after_suite trying to build an authenticated connection. Now: - All test definitions store under a per-run prefix, uploads/<run id> (GCSTest.Run). The id comes from WAFFLE_TEST_RUN_ID (CI sets it to something job-unique) or is generated per local run. URL assertions derive from the same helper. - Cleanup lists only that prefix (in both configured buckets) and is skipped when :integration is excluded or GCP_CREDENTIALS is absent. - Goth only starts in test_helper when credentials are present, so mix test.unit is genuinely offline: no creds, no network. - util_test restored WAFFLE_BUCKET with put_env(key, nil), which crashes when the var was never set - visible for the first time now that credential-less runs exist. Runs that crash before cleanup leave their prefix behind; the README recommends a short lifecycle TTL on test buckets to sweep those. Verified: mix test.unit green with no GCS-related env vars at all; full suite green against real GCS; both buckets list zero objects under uploads* afterwards.
Closes #36. The single job ran the full suite - integration tests included - with secrets on every PR. Fork PRs don't receive secrets, so any outside contribution failed on auth rather than on its merits. Now: - unit: mix test.unit across the version matrix, plus compile --warnings-as-errors and the format check. No secrets referenced, so it runs (and can pass) everywhere, fork PRs included. MIX_ENV=test is explicit on the test step because def cli only exists on Elixir >= 1.15. - integration: the full suite against real GCS on one modern Elixir/OTP pair, skipped for fork PRs where secrets are absent. Passes WAFFLE_BUCKET2 through (needed by the bucket-from-scope test once #33 lands) and sets WAFFLE_TEST_RUN_ID from the workflow run id and attempt, so concurrent runs sharing the test buckets stay isolated and the after-suite cleanup stays scoped to this run. - workflow_dispatch trigger for manual runs.
Three fixes from the first run of the split workflow (PR #42): - ubuntu-24.04 runner images no longer ship ImageMagick, so the integration job installs it before running the transform tests. - Formatter output is only stable within an Elixir version (1.15 wraps lines 1.17 accepts), so the format check now runs once, on the newest matrix entry, and the unit matrix gets fail-fast: false so one version's failure doesn't cancel the others' results. - Run mix format properly: the earlier local check piped through tail, which hid a real failure. Shorten the regression test's tag string while at it.
Collaborator
Author
|
@ulissesalmeida I combined all the bits from #26 around the filename fix so it works for all the operations, then added some tests and plan files to go along with the issues/milestones. I'm probably going to merge in the next couple days unless you wanted to take a look |
ulissesalmeida
left a comment
ulissesalmeida
left a comment
Collaborator
There was a problem hiding this comment.
Thank you for including me
Although I changed jobs a year ago, so it has been years I don't need to use this library 😅 So I think I'm not the best person anymore to review it.
I did a quick scan, I couldn't see anything obviously bad, so follow your ❤️
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
2 participants
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.
Forward-ports the double-filename fix and the rebuilt test suite from the v0.2.x line onto
main, plus the first two prioritized v0.3 infrastructure fixes. This is step 0 of the v0.3 plan — seeplans/README.md(added here) for the full roadmap and the v0.3 milestone.Fixes #25. Closes #35. Closes #36.
What's here
The lib fix (#25):
put/3joins the storage dir withfile.file_nameas-is — Waffle has already resolved the version filename before calling the adapter — whilepath_for/3keeps its resolving semantics (via a newfullname/3wrapper) fordelete/3and the URL builders, which receive the original client filename and must resolve it exactly once. This differs from PR #1, which stoppedpath_for/3from resolving at all. A regression test drivesstorethrough a probe definition and assertsfilename/2is invoked exactly once per version.Note for direct callers of
CloudStorage.put/3(not viadefinition.store): filename resolution is no longer applied insideput— pass the final object name infile.file_name.The test overhaul: feature-organized integration suite (
test/integration/), single-purpose test definitions, shared case template with assertion macros, tag taxonomy (documented intest/test_helper.exs), and amix test.unitalias for offline runs.#35 — cleanup safety: all test objects live under a per-run prefix (
uploads/<run id>, fromWAFFLE_TEST_RUN_IDor generated); the after-suite cleanup deletes only that prefix (never the whole bucket) and is skipped for offline or credential-less runs.mix test.unitnow needs no credentials and no network.#36 — CI split: a secret-free
unitjob (full version matrix, runs on fork PRs too) and a guardedintegrationjob (one modern pair, real GCS, skipped where secrets don't exist).WAFFLE_BUCKET2is passed through for #33, andWAFFLE_TEST_RUN_IDis set from the workflow run so concurrent runs share the test buckets safely.Plans: the
plans/directory (excluded from the hex package) with the 0.3 release plan, offline-test-suite plan, client-rewrite plan, and migration/repair plan.Verification
mix test.unitwith zero GCS-related env vars: 56 tests, 0 failures, clean exit.put/3fails the regression test; restoring the fix passes it.uploads*after the suite.mix hex.build:plans/absent from the package.First-run notes
convert) is preinstalled on ubuntu-latest (it is, per GitHub's runner images); if that ever changes it needs a one-line apt-get step.plans/0.3-release.md§4).