Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add set preset #139

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

sagudev
Copy link
Collaborator

@sagudev sagudev commented Jul 31, 2024

Sets only reported results.

This is useful for partial runs (when only subset of tests are run). This is mostly useful for local development when focusing on fixing specific tests, I used this in servo a lot.

From #80

Sets only reported results (useful for partial runs)

Signed-off-by: sagudev <[email protected]>
Copy link
Collaborator

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: This is basically a modified ReportProcessingPreset::ResetAllOutcomes (RE: #139 (comment)). I'm not firmly settled in this opinion, but I suspect that an option that overrides this particular preset behavior, rather than a new preset, would be an easier CLI to understand (as a user) and maintain.

My suggestion is to resurrect --on-missing-from-report={delete,reconcile}-{silently,and-{info,warn}}]; I'm happy to implement that myself. I wonder if a --partial-run (name TBD) flag might be more clear WRT intent, likely as an alias (i.e. for --on-missing-from-report=keep-and-info or keep-silently).

@ErichDonGubler ErichDonGubler self-assigned this Jul 31, 2024
Comment on lines +140 to +143
ReportProcessingPreset::SetNewOutcomes => |meta, rep| match rep {
Some(rep) => rep,
None => meta,
},
Copy link
Collaborator

@ErichDonGubler ErichDonGubler Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: At first glance, this appears to be different from ResetAll WRT fallback (i.e., Set falls back to meta, rather than ResetAll's Default::default()). But as of #131, ResetAll actually doesn't ever run into the fallback case, because missing entries are deleted and skip reconciliation entirely. Huh...I hadn't considered that that wasn't exercised before. 😅

We probably want to change ResetAll to match this function body instead. 🤔 If we do, I think that strengthens the argument for a separate option (instead of a new preset) at #139 (review).

@ErichDonGubler ErichDonGubler added the enhancement New feature or request label Jul 31, 2024
@sagudev
Copy link
Collaborator Author

sagudev commented Jul 31, 2024

I saw #25, but given the last comment I though separate preset was ok. I can also do --on-missing-from-report as this is the most flexible solution.

So presets determine how reconciliation happens with reported results (Some) and --on-missing-from-report would determinate what to do in None cases?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants