RFC: Migrate Test Framework from sclevine/spec#341
Open
jkutner wants to merge 2 commits into
Open
Conversation
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
Member
|
Maintainers, As you review this RFC please queue up issues to be created using the following commands: Issues(none) |
Member
|
@jkutner are you wanting to scope this just to |
Member
Author
|
@hone i want to eventually hit them all, but thought i might start small. do you think I should reframe the RFC to be about all of them? or do an experiment first? |
Member
Author
|
Here's a Draft PR for Phase 1 |
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
Member
Author
|
@hone @jabrown85 fyi updated to expand scope to all buildpacks org repos that use |
hone
reviewed
Jul 10, 2026
| }) | ||
| ``` | ||
|
|
||
| **Decision criteria**: If any `it()` within the scope mutates the state set up by `it.Before()`, use Option A. If all `it()` blocks only read the state, Option B is safe and more concise. |
Member
There was a problem hiding this comment.
sounds like we're going to employ both of these depending on the decision criteria/
hone
reviewed
Jul 10, 2026
|
|
||
| **Disadvantages:** | ||
| - `SetupTest()` runs for ALL test methods in a suite, creating a fundamental mismatch with scoped `it.Before()`. The 223 scoped setup blocks either require proliferating many small suite structs per file (e.g., `RebaserForceSuite`, `RebaserNoForceSuite`) or using `s.Run()` subtests with inline setup -- which negates the suite benefit. | ||
| - Introduces an OOP mental model (struct fields, method receivers, embedding) that is not idiomatic Go testing. |
Member
There was a problem hiding this comment.
what is the idiomatic way to do this?
hone
reviewed
Jul 10, 2026
|
|
||
| 3. **What is the threshold for choosing Option A (per-test setup function) vs. Option B (top-of-closure setup)?** A clear guideline is needed for the 223 scoped `it.Before` blocks to ensure consistency across the codebase. | ||
|
|
||
| 4. **Should the AST transformation tool be contributed to the community?** Other projects outside the buildpacks organization that use `sclevine/spec` may benefit from a migration tool. Within the organization, the tool will live in `lifecycle/tools/spec-migrate` during the migration and be removed once all repos are converted (or, alternatively, promoted to a small standalone repo before removal). |
Member
There was a problem hiding this comment.
i think it's fine to just make a repo for it in the buildpacks org.
hone
reviewed
Jul 10, 2026
|
|
||
| 5. **How should the `each()` helper in `buildpack/build_test.go` (lifecycle) be restructured?** Table-driven tests are the most idiomatic approach but change the test's conceptual structure. Similar helpers in other repos follow the same decision. | ||
|
|
||
| 6. **What order should the repos be migrated in?** Lifecycle is the natural first target because it exercises every pattern (deep nesting, scoped setup, custom helpers). After lifecycle, order is flexible; small repos (`registry-api`, `imgutil`) offer quick wins, while `pack` benefits most from the settled tooling. |
Member
There was a problem hiding this comment.
does it make sense to start with a small repo first?
hone
approved these changes
Jul 10, 2026
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
3 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.
Readable