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

Change verbosity of shrink disabled and persisted failure messages #453

Open
grandizzy opened this issue May 23, 2024 · 3 comments
Open
Labels
quality-of-life This issue proposes a change that will improve the UX of proptest but isn't necessarily a "feature"

Comments

@grandizzy
Copy link

I am working to implement an (indicatif based) progress bar for tests that use proptest and I need to suppress couple of messages that I really think should follow the info verbose rule (as they have to be explicitly configured, so not unexpected errors per se). Please let me know if you'd be open for a PR with changes below. Thank you!

  • when shrink is disabled: do not start loop and exit early in shrink fn
    fn shrink<V: ValueTree>(
    &mut self,
    case: &mut V,
    test: impl Fn(V::Value) -> TestCaseResult,
    replay_from_fork: &mut impl Iterator<Item = TestCaseResult>,
    result_cache: &mut dyn ResultCache,
    fork_output: &mut ForkOutput,
    is_from_persisted_seed: bool,
    ) -> Option<Reason> {
    #[cfg(feature = "std")]

    with a info message (since disabling should be explicitly set in Config as max_shrink_iters: 0), proposed addition :
         fork_output: &mut ForkOutput,
         is_from_persisted_seed: bool,
     ) -> Option<Reason> {
+        if self.config.max_shrink_iters() == 0 {
+            verbose_message!(
+                self,
+                INFO_LOG,
+                "Shrinking disabled by configuration"
+            );
+            return None
+        }
+
  • when failure is persisted: apply same INFO_LOG level to persist message below (since failure_persistence should explicitly be set in Config for recording failures)
    eprintln!(
    "proptest: Saving this and future failures in {}\n\
    proptest: If this test was run on a CI system, you may \
    wish to add the following line to your copy of the file.{}\n\
    {}",
    path.display(),
    if is_new { " (You may need to create it.)" } else { "" },
    seed);
@grandizzy
Copy link
Author

ping @matthew-russo any insights highly appreciated. thank you!

@matthew-russo
Copy link
Member

Sorry for the delay, I had to step away for the past couple months to deal with some things at home. I'll be back working on things starting this weekend and will start triaging the issues that have come in.

I think the first suggestion is non-controversial and easy to accept.

I think the second suggestion isn't as straightforward since its only logging because there was a failure (hence its in the context of an error) and its anticipating that it may be running in a CI system, where users may only keep stdout. How are you going about suppressing it right now?

@matthew-russo matthew-russo added the quality-of-life This issue proposes a change that will improve the UX of proptest but isn't necessarily a "feature" label Jun 8, 2024
@grandizzy
Copy link
Author

Thank you! There's no suppression rn so basically the progress bar gets flaky when proptest prints messages. I will make the PR for first case, what if for 2nd one we add an env setting that if turned on (default off) would skip printing such messages?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality-of-life This issue proposes a change that will improve the UX of proptest but isn't necessarily a "feature"
Projects
None yet
Development

No branches or pull requests

2 participants