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 --fail-fast #455

Merged
merged 37 commits into from
Oct 19, 2024
Merged

Add --fail-fast #455

merged 37 commits into from
Oct 19, 2024

Conversation

tgdwyer
Copy link
Contributor

@tgdwyer tgdwyer commented Sep 30, 2024

Added a command line option "--stop-on-fail".
When in --stop-on-fail mode, after a test in an example group fails, running tests from subsequent example groups is aborted.

@sol sol changed the title implement stop on fail mode Add --fail-fast Sep 30, 2024
Copy link
Owner

@sol sol left a comment

Choose a reason for hiding this comment

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

Can you please try the following: Add a test case with a failing $setup and run it with --fail-fast, then observe if it will abort early. My suspicion is no, it won't.

To address this you may need to add shortcut semantics to the stack (e.g. add MaybeT to the Report type).

Ideally this will allow you to handle early abortion in reportFailure / reportError.

src/Options.hs Outdated Show resolved Hide resolved
src/Options.hs Outdated Show resolved Hide resolved
src/Options.hs Outdated Show resolved Hide resolved
src/Options.hs Show resolved Hide resolved
test/OptionsSpec.hs Outdated Show resolved Hide resolved
src/Run.hs Outdated Show resolved Hide resolved
test/integration/failing-multiple-groups/Foo.hs Outdated Show resolved Hide resolved
@sol
Copy link
Owner

sol commented Sep 30, 2024

Regarding the failing CI job, you'll need to run hpack to update the .cabal-file.

@sol
Copy link
Owner

sol commented Sep 30, 2024

@tgdwyer just to be sure, because the tests take a long time to run, you can focus on individual test cases by changing it to fit (or alternatively focus on a group of test cases by changing describe to fdescribe. This is usually more convenient then passing --match on the command line.

https://hackage.haskell.org/package/hspec-2.11.9/docs/Test-Hspec.html#g:5

@tgdwyer tgdwyer closed this Oct 2, 2024
@tgdwyer tgdwyer deleted the stopOnFail branch October 2, 2024 04:19
@tgdwyer tgdwyer restored the stopOnFail branch October 2, 2024 04:36
@tgdwyer tgdwyer reopened this Oct 2, 2024
@tgdwyer
Copy link
Contributor Author

tgdwyer commented Oct 2, 2024

I have renamed the option to --fail-fast and made the various merges and cleanups.

I did some experimenting with $setup and it looks like it fast fails already. For the following test:

module Foo where

-- $setup
-- >>> let x = 1 + "hello"

-- | foo
-- >>> foo
-- 42
foo :: Int
foo = 42

doctest (whether with --fast-fail or not) produces:

test/extract/setup/Foo.hs:4: failure in expression `let x = 1 + "hello"'
expected:
but got:
^
:27:9: error:
• No instance for (Num String) arising from the literal ‘1’
...
Examples: 2 Tried: 1 Errors: 0 Failures: 1

Meanwhile, I agree it would probably better to use MaybeT to respond to fast fail than just looking for failures in the Report counts. I would like to investigate this, but won't have time for a few weeks. Any such refactoring would probably need to be part of a separate PR.

Anything else?

@tgdwyer tgdwyer requested a review from sol October 2, 2024 08:48
@sol
Copy link
Owner

sol commented Oct 2, 2024

I did some experimenting with $setup and it looks like it fast fails already.

I think what's happening here is that, if $setup fails, all examples from the current module will be skipped. My suspicion is that it would still continue with other modules. So to reproduce the "issue" you would need a test case with multiple modules.

I somehow missed that on my first review.

@tgdwyer
Copy link
Contributor Author

tgdwyer commented Oct 4, 2024

My suspicion is that it would still continue with other modules. So to reproduce the "issue" you would need a test case with multiple modules.

I've changed the logic of runModule to skip if failures > 0 (rather than only on increasing failure count). Thus, after the first failure all subsequent examples will be skipped regardless of module.
I've added another test case with a second file to confirm its module is skipped.

I've also run hpack so the test files are included in doctest.cabal and the CI should run.

src/Options.hs Show resolved Hide resolved
@tgdwyer
Copy link
Contributor Author

tgdwyer commented Oct 7, 2024

I think I've implemented all the requested changes and that it's ready for merge, if you're happy?

Copy link
Owner

@sol sol left a comment

Choose a reason for hiding this comment

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

I think this is still not working correctly if you have multiple $setup blocks (see my other comment).

Would you mind cherry-picking bbe00c6 and using this as a starting point?

src/Options.hs Outdated Show resolved Hide resolved
src/Options.hs Outdated Show resolved Hide resolved
src/Options.hs Outdated Show resolved Hide resolved
src/Options.hs Show resolved Hide resolved
src/Options.hs Show resolved Hide resolved
src/Run.hs Show resolved Hide resolved
test/OptionsSpec.hs Outdated Show resolved Hide resolved
test/OptionsSpec.hs Outdated Show resolved Hide resolved
test/OptionsSpec.hs Outdated Show resolved Hide resolved
src/Runner.hs Outdated Show resolved Hide resolved
@sol
Copy link
Owner

sol commented Oct 15, 2024

@tgdwyer also cherry-pick b4a777c, or just rebase on https:/sol/doctest/tree/maybe-t.

@tgdwyer
Copy link
Contributor Author

tgdwyer commented Oct 18, 2024

@tgdwyer also cherry-pick b4a777c, or just rebase on https:/sol/doctest/tree/maybe-t.

Done! Thanks for wrapping Report in MaybeT! FailFast logic is much better now. Latest commits just use forM_ to traverse the Maybe effect in runModule, same as runModules. Code is much cleaner.

@tgdwyer tgdwyer requested a review from sol October 18, 2024 05:28
Copy link
Owner

@sol sol left a comment

Choose a reason for hiding this comment

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

Great, the code looks good now 👍

Let's clean up the test descriptions, and then it's good to go.

In addition, I think you need to run hpack again.

test/OptionsSpec.hs Outdated Show resolved Hide resolved
test/MainSpec.hs Outdated Show resolved Hide resolved
test/MainSpec.hs Outdated Show resolved Hide resolved
test/MainSpec.hs Outdated Show resolved Hide resolved
test/MainSpec.hs Outdated Show resolved Hide resolved
test/MainSpec.hs Outdated Show resolved Hide resolved
@sol sol enabled auto-merge (squash) October 19, 2024 01:42
@sol sol merged commit f27509c into sol:main Oct 19, 2024
47 of 50 checks passed
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.

2 participants