fix: use panic unwind on tests with quick profile#6621
Conversation
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds a new Cargo profile "quick-test", updates mise task defaults/choices to expose it, and adjusts the CI workflow to invoke mise without the previous explicit "quick" argument so the new default profile is used. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mise.toml (1)
155-156: Consider defaulting test tasks toquick-test.Default remains
quick, somise teststill uses panic=abort unless the caller passesquick-test. If the goal is to avoid aborting panics by default, switching the default toquick-test(or explicitly documenting this in dev docs) would align local runs with CI.Suggested diff (apply similarly to other test tasks)
-arg "<profile>" help="Build profile (quick-test, dev, etc.)" default="quick" { +arg "<profile>" help="Build profile (quick-test, dev, etc.)" default="quick-test" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mise.toml` around lines 155 - 156, Change the default build profile for the test arg so local `mise test` uses the non-aborting test profile: update the arg declaration for "<profile>" (the argument block currently with help="Build profile (quick-test, dev, etc.)" and choices "quick" "quick-test" "release" "dev") to set default="quick-test" instead of default="quick" (apply same change to other test task arg blocks if present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mise.toml`:
- Around line 178-180: The arg declaration for profile (arg "<profile>"
default="quick") duplicates an earlier default-profile suggestion — update this
arg's default to the same recommended default used elsewhere (replace
default="quick" with the agreed-upon value) and ensure the choices list on arg
"<profile>" remains correct; locate the arg "<profile>" block and change only
the default to match the prior recommendation so both declarations are
consistent.
- Around line 166-168: The arg "<profile>" block currently sets default="quick"
but the review notes the default profile should match the project's standard
(duplicate suggestion applies here); update the arg "<profile>" declaration (the
arg "<profile>" with choices "quick" "quick-test" "release" "dev") to use the
consistent default used elsewhere (e.g., change default from "quick" to
"quick-test" or whatever the canonical default is) so the default aligns with
other profile arg definitions.
- Around line 190-192: The arg block for "<profile>" currently sets
default="quick" but the reviewer notes the same default-profile suggestion
should be applied here; update the arg "<profile>" declaration (the block
defining arg "<profile>" help="Build profile (quick-test, dev, etc.)"
default="quick") to use the agreed default (e.g., default="quick-test") and
ensure the choices list ("quick" "quick-test" "release" "dev") still includes
that default; also verify any other "<profile>" arg occurrences are consistent
with this default to avoid duplicate/conflicting defaults.
---
Nitpick comments:
In `@mise.toml`:
- Around line 155-156: Change the default build profile for the test arg so
local `mise test` uses the non-aborting test profile: update the arg declaration
for "<profile>" (the argument block currently with help="Build profile
(quick-test, dev, etc.)" and choices "quick" "quick-test" "release" "dev") to
set default="quick-test" instead of default="quick" (apply same change to other
test task arg blocks if present).
5fe3f0b to
de2be4d
Compare
de2be4d to
292b427
Compare
Summary of changes
quickprofile which inheritspanic=abortresults in extremely cryptic errors when, e.g., trying to override packages infil-actor-states. See https://github.com/ChainSafe/fil-actor-states/actions/runs/21994597700/job/63550945119?pr=413panic=abortis also undesirable in tests for various reasons (should_panictest would be incorrectly handled I guess, also worse error messages).Changes introduced in this pull request:
quick-testprofile that doesn't abort on paics.Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit