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 option to skip printing skipped tests + fix FsCheck3 replay #511

Closed
wants to merge 6 commits into from

Conversation

rynoV
Copy link
Contributor

@rynoV rynoV commented Sep 10, 2024

Resolves #453 : 79f6e21

Also fixes the replay config so fscheck 3 will skip directly to the failing case: https://github.com/haf/expecto/pull/511/files/79f6e21881ae566036a22bbf0f1879454bf5784a..701746a1b3a2773ab9a011b221ce15e2f9345d0e

@rynoV rynoV changed the title Add option to skip printing skipped tests Add option to skip printing skipped tests + fix FsCheck3 replay Sep 10, 2024
@farlee2121
Copy link
Collaborator

I probably won't be able to review this for a few weeks.

@farlee2121
Copy link
Collaborator

Alright. I'm back and starting to give this a look.

I think these are distinct work items that should be considered separately. Trying to review them together muddies the picture of what's going on with either

@farlee2121
Copy link
Collaborator

farlee2121 commented Oct 13, 2024

I see motivations for skipping skip messages in #453.
Could you explain the perceived fix to FsCheck replay?

@rynoV
Copy link
Contributor Author

rynoV commented Oct 15, 2024

Yeah my bad should have made two PRs. I updated the description with links to the diffs for the two changes

Could you explain the perceived fix to FsCheck replay?

In FsCheck3 they seem to have added (or maybe factored out) a size parameter for replay in addition to the seed. Without the size, replay was playing all test inputs leading up to the failure; this change allows skipping directly to the failing test input

@farlee2121
Copy link
Collaborator

That makes sense. Skipping right to the broken case would be good for debugging.

I noticed that you also changed from printing the original seed to the final seed.
I'm unsure how that impacts users focused more on deterministically repeating a specific run.
Expecto itself has some tests that use the seed that way

@rynoV
Copy link
Contributor Author

rynoV commented Oct 17, 2024

Ah didn't think about that. I added it to the output

@farlee2121
Copy link
Collaborator

farlee2121 commented Dec 29, 2024

This fell off my radar. Apologies.

I would need these work items to be split before merging either.
A clean is history for future context is very important for a community maintained package like this.

From what I've see so far

  • The Replay changes seem good
  • Not printing skipped test messages has me a bit more worried. There's already some challenging interactions/interdependence between the config and the printers. I'll need to consider it and alternative approaches more carefully

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.

Option to suppress "Skipped" messages
2 participants