Various `cabal-testsuite` improvements #10225

jasagredo · 2024-07-23T22:24:49Z

Make .bat scripts for CCompilerOverride test look into the shipped toolchain for gcc/clang.
The flaky combinator can be used to specify flaky tests with a ticket number. These will be reported as passing or failing but will not make the test-suite error.
Broken tests can be broken by accept-test, i.e. by the output not matching. Previously a test that failed this way had to be skipped because it didn't pass but the expectBroken logic would deem it not broken.
Also makes postCheckoutCommand not flaky on Windows.
It removes some outdated tests (old GHCs) and some tests that will never pass again.
Fixes some issues with output normalization in MacOS

Depends-On: #10282

cabal-testsuite/src/Test/Cabal/Monad.hs

cabal-testsuite/src/Test/Cabal/TestCode.hs

jasagredo · 2024-07-24T17:16:49Z

Hm what a surprising failure on macOS! Will investigate

cabal-testsuite/src/Test/Cabal/TestCode.hs

cabal-testsuite/src/Test/Cabal/Monad.hs

ulysses4ever · 2024-07-24T18:39:22Z

cabal-testsuite/src/Test/Cabal/Prelude.hs

+skipIfOSX :: String -> IO ()
+skipIfOSX why = skipIfIO ("OSX " <> why) isOSX
+
+skipIfCI :: Int -> IO ()
+skipIfCI ticket = skipIfIO ("CI, see #" <> show ticket) =<< isCI
+
+skipIfCIAndWindows :: Int -> IO ()
+skipIfCIAndWindows ticket = skipIfIO ("Windows CI, see #" <> show ticket) . (isWindows &&) =<< isCI
+
+skipIfCIAndOSX :: Int -> IO ()
+skipIfCIAndOSX ticket = skipIfIO ("OSX CI, see #" <> show ticket) . (isOSX &&) =<< isCI
+
+expectBrokenIfWindows :: Int -> TestM a -> TestM a
+expectBrokenIfWindows ticket = expectBrokenIf isWindows ticket
+
+expectBrokenIfWindowsCI :: Int -> TestM a -> TestM a
+expectBrokenIfWindowsCI ticket m = do
+    ci <- liftIO isCI
+    expectBrokenIf (isWindows && ci) ticket m
+
+expectBrokenIfWindowsCIAndGhc :: String -> Int -> TestM a -> TestM a
+expectBrokenIfWindowsCIAndGhc range ticket m = do
+    ghcVer <- isGhcVersion range
+    ci <- liftIO isCI
+    expectBrokenIf (isWindows && ghcVer && ci) ticket m
+
+expectBrokenIfWindowsAndGhc :: String -> Int -> TestM a -> TestM a
+expectBrokenIfWindowsAndGhc range ticket m = do
+    ghcVer <- isGhcVersion range
+    expectBrokenIf (isWindows && ghcVer) ticket m
+
+expectBrokenIfOSXAndGhc :: String -> Int -> TestM a -> TestM a
+expectBrokenIfOSXAndGhc range ticket m = do
+    ghcVer <- isGhcVersion range
+    expectBrokenIf (isOSX && ghcVer) ticket m
+
+expectBrokenIfGhc :: String -> Int -> TestM a -> TestM a
+expectBrokenIfGhc range ticket m = do
+    ghcVer <- isGhcVersion range
+    expectBrokenIf ghcVer ticket m
+
+flakyIfCI :: Int -> TestM a -> TestM a
+flakyIfCI ticket m = do
+    ci <- liftIO isCI
+    flakyIf ci ticket m
+
+flakyIfWindows :: Int -> TestM a -> TestM a
+flakyIfWindows ticket m = flakyIf isWindows ticket m


I am not sure I buy it as an improvement having separate functions for so many combinations. It'd be great to have a little DSL for expressing such things and this looks like a step away from that. But it is tempting to go this route because it does save on typing. Perhaps if Haskell had a built-in the monadic bang (in the sense of this plugin), the savings here wouldn't be as noticeable. So, overall, I don't feel strongly about it, just curious what others think.

Yeah, as I was writing them I was thinking the same. But overall I think it is useful to make the skip and broken messages as uniform as possible, to be able to grep the output.

Otherwise one might use "shared libs", another person uses "dynamic libs", another one makes a typo "shred libs", etc.

I will consider whether to add something like a DSL. But maybe this would be mergeable? At the very least I could put a comment in the code saying "TODO: this should be reworked into a DSL or some such".

You don't need to risk loss of uniformity. See for example https://hackage.haskell.org/package/xmonad-contrib-0.18.0/docs/XMonad-Util-WindowProperties.html.
In this case I'd have 4 parameters: expected state or action (skip, expect fail, expect pass, flaky…), predicate (cf. above WindowProperties, replacing your multiple conditionals), ticket, message. But this might indeed be future work.

Ah i didn't explain myself. I think a DSL would be uniform and the best solution. What is not uniform is

skipIf noShared "no shared libs"

And somewhere else

skipIf noShared "no dyn libs"

Which is what was there before I introduced these functions.

I would make that aspect of the message come from the DSL, and the explanation should give details. (In which case perhaps it should be optional, or absorbed into the ticket.)

Ok I managed to make the CI green. From my side there are no other blockers for merging this.

What is your opinion on the combinators then @ulysses4ever @geekosaur ? Could this be merged and the DSL be done in a later PR or do you think this should not be merged before the DSL is done?

By no means I meant that the DSL should be a prerequisite.

I simply struggle to get to the bottom of the long line of commits. For me personally, it'd be much easier to approve commits in separate PRs one by one. But I understand how inconvenient that approach is for the author. So I'll try to get a review done.

Some points I wanted to note down:

It would have been perfectly ok if you said you wanted the DSL before approving, it is a perfectly valid opinion and I asked because I would have thought it was perfectly ok if you said "I will not give my approval until this PR has better ergonomics for the combinators like a DSL maybe"

I have tried to split the changes semantically, one on each commit, so that it would be easier to review. Some of them are interesting, some are boring 😄

(Btw, while reviewing you can choose to see only one commit at a time, that might make it easier)

I'm fine with splitting the work in multiple PRs if it is independent (kind of like in this case, it could have been 4-5 PRs) but I think it is more work to drag attention to 5 PRs for review than to one. There is usually not enough volunteer-time to review this so I prefer just making it in one go 🙂 (as long as the commits are related, like in this case they are all about CI and tests)

This being said, if you prefer I can split this PR in multiple PRs.

I think I already suggested it should be future work? It wants some up-front design.

For splitting: I think you should do it the way you prefer. For me personally reviewing and approving 5 small PRs can take much less time than reviewing 5 commits and approving one PR with those. That's simply because it's not possible to "save state" (meaning my mental state) in between the commits while there's a way to do that for multiple PRs (by approving them one by one). Unless I'm missing something in GitHub UI. I know that you can review commits separately (that's what I've been doing with this PR), but that's not the same as reviewing/approving/and forgetting about individual PRs (which works better for me). Again, you should do it the way you find it more convenient, I think.

For the DSL, I share Brandon's opinion (see the comment just above) entirely.

jasagredo · 2024-07-25T16:19:58Z

I think I fixed the MacOS issue with 4285454. Let's see what CI says, then I will address the review comments.

For the DSL, I tried doing something last night but things started looking very bad as there were just too many edge cases and conditions applied in different places. I will give it some more thought.

cabal-testsuite/README.md

jasagredo · 2024-07-26T07:45:55Z

The RejectFutureIndexStates test is timing-out on Windows on CI. This doesn't happen on my local machine, which is kind of weird.

I will probably have to mark it as Skip on Windows+CI 🙄 or investigate with the tmate action.

jasagredo · 2024-07-26T15:14:26Z

I deferred the investigation of those two tests to #10230. Before this PR they were skipped anyway on all OSes.

Cabal-tests/lib/Test/Utils/TempTestDir.hs

cabal-testsuite/src/Test/Cabal/Monad.hs

jasagredo · 2024-07-29T15:51:33Z

May I ask for an approval if there are no further comments @ulysses4ever ? (Just pinging you because you had reviewed the pr already, I can ask in matrix if you can't review it)

ulysses4ever · 2024-07-29T16:02:14Z

@jasagredo it's on my radar, yes. In the meantime, applying the needs-review label (just did it) and asking on Matrix may speed it up. I can't guarantee that I get to it today but I'll try my best.

ulysses4ever · 2024-07-30T02:49:58Z

@mpickering I think you were one of the last people to do a meaningful update to the Cabal testsuite. Could you, perhaps, take a look at this PR?

ulysses4ever · 2024-07-30T02:52:47Z

cabal-testsuite/main/cabal-tests.hs

+            (e, out, err) <- readProcessWithExitCode real_path real_args ""
+            putStrLn "# STDOUT:"
+            putStrLn out
+            putStrLn "# STDERR:"
+            putStrLn err
+            if "TestCodeFlaky" `isInfixOf` err
+              then pure ()
+              else throwIO e


This grepping through stderr feels a little crude although I don't know a better way. Also, usually, when you switch from showing direct output to showing captured one, there are some side effects. E.g. colored output would be unavailable, etc. Of course, Cabal testsuite doesn't have colored output in particular but there may be spooky actions at the distance...

I thought maybe there was some other way to pipe this output of the process but I didn't seem to find one.

In any case this is only for when you invoke the tests with one test. I don't know how much that is used, I personally run the validate script always

this is only for when you invoke the tests with one test

Oh, that's much less of a concern then. Thank you!

Let's leave it as unresolved for now maybe to see if anyone has other thoughts...

jasagredo · 2024-08-28T14:53:33Z

Let's wait until the Windows CI is reenabled before merging this

geekosaur · 2024-08-28T16:38:01Z

I set a dependency so Mergify should merge it automatically when #10282 goes in.

These tests were testing for messages that were removed with the `cabal check` rework.

This was broken in haskell#10225

geekosaur approved these changes Jul 23, 2024

View reviewed changes

jasagredo marked this pull request as draft July 23, 2024 23:55

jasagredo force-pushed the js/flaky-tests branch from 78c3ab3 to d99812b Compare July 23, 2024 23:58

jasagredo commented Jul 24, 2024

View reviewed changes

cabal-testsuite/src/Test/Cabal/Monad.hs Outdated Show resolved Hide resolved

cabal-testsuite/src/Test/Cabal/TestCode.hs Outdated Show resolved Hide resolved

jasagredo changed the title ~~Implement flaky combinator~~ Implement flaky combinator Jul 24, 2024

jasagredo mentioned this pull request Jul 24, 2024

Remove read-only mark on dist-newstyle when doing cabal clean on Windows #10190

Merged

5 tasks

jasagredo force-pushed the js/flaky-tests branch from d99812b to f364868 Compare July 24, 2024 15:21

jasagredo marked this pull request as ready for review July 24, 2024 15:31

ulysses4ever reviewed Jul 24, 2024

View reviewed changes

cabal-testsuite/src/Test/Cabal/TestCode.hs Outdated Show resolved Hide resolved

ulysses4ever reviewed Jul 24, 2024

View reviewed changes

cabal-testsuite/src/Test/Cabal/Monad.hs Show resolved Hide resolved

ulysses4ever reviewed Jul 24, 2024

View reviewed changes

jasagredo force-pushed the js/flaky-tests branch from 4285454 to 431bf9f Compare July 25, 2024 21:17

jasagredo mentioned this pull request Jul 25, 2024

Improve bat scripts for CCompilerOverride #10224

Closed

jasagredo changed the title ~~Implement flaky combinator~~ Various cabal-testsuite improvements Jul 25, 2024

jasagredo commented Jul 25, 2024

View reviewed changes

cabal-testsuite/README.md Show resolved Hide resolved

jasagredo force-pushed the js/flaky-tests branch from 431bf9f to 82cb43c Compare July 25, 2024 22:09

jasagredo force-pushed the js/flaky-tests branch 2 times, most recently from 54f1d18 to b25a3ca Compare July 26, 2024 15:40

ulysses4ever reviewed Jul 26, 2024

View reviewed changes

Cabal-tests/lib/Test/Utils/TempTestDir.hs Outdated Show resolved Hide resolved

jasagredo commented Jul 26, 2024

View reviewed changes

cabal-testsuite/src/Test/Cabal/Monad.hs Show resolved Hide resolved

jasagredo force-pushed the js/flaky-tests branch from b25a3ca to 6cd9030 Compare July 28, 2024 15:48

ulysses4ever added the attention: needs-review label Jul 29, 2024

ulysses4ever reviewed Jul 30, 2024

View reviewed changes

jasagredo force-pushed the js/flaky-tests branch from 66beb26 to d3f9c29 Compare August 26, 2024 21:59

jasagredo added the squash+merge me Tell Mergify Bot to squash-merge label Aug 26, 2024

jasagredo force-pushed the js/flaky-tests branch from d3f9c29 to 2698f8c Compare August 27, 2024 21:35

mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Aug 28, 2024

jasagredo removed squash+merge me Tell Mergify Bot to squash-merge ready and waiting Mergify is waiting out the cooldown period labels Aug 28, 2024

geekosaur added the squash+merge me Tell Mergify Bot to squash-merge label Aug 28, 2024

mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Aug 28, 2024

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Aug 30, 2024

jasagredo force-pushed the js/flaky-tests branch from 2698f8c to d687371 Compare September 1, 2024 21:03

jasagredo added 5 commits September 2, 2024 00:07

Improve bat scripts for CCompilerOverride

c1066ca

Ensure Windows tests can cleanup the temp directory

6a02f09

Implement flaky combinator

39d6c1c

Remove outdated tests

64c7e4a

Remove broken tests

993d452

These tests were testing for messages that were removed with the `cabal check` rework.

jasagredo force-pushed the js/flaky-tests branch from d687371 to 238c1f0 Compare September 1, 2024 22:08

jasagredo added 6 commits September 2, 2024 00:16

Make skip and broken messages uniform

c0af542

Mark flaky tests

dc8d60f

Re-enable DeterministicTrivial

fb21b24

Fix MacOS canonical paths

d575fea

Extend cabal-testsuite readme with flaky

fd74788

Skip non-terminating tests in Windows CI

e14eacd

jasagredo force-pushed the js/flaky-tests branch from 238c1f0 to e14eacd Compare September 1, 2024 22:16

Merge branch 'master' into js/flaky-tests

72040bc

mergify bot merged commit 39b6924 into haskell:master Sep 2, 2024
42 checks passed

mergify bot mentioned this pull request Sep 5, 2024

Fix Windows tests depending on scripts (backport #10236) #10319

Merged

2 tasks

9999years added a commit to 9999years/cabal that referenced this pull request Sep 25, 2024

Fix --accept flag in cabal-testsuite

0685cb3

This was broken in haskell#10225

9999years mentioned this pull request Sep 25, 2024

Fix --accept flag in cabal-testsuite #10382

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various `cabal-testsuite` improvements #10225

Various `cabal-testsuite` improvements #10225

jasagredo commented Jul 23, 2024 •

edited by geekosaur

Loading

jasagredo commented Jul 24, 2024

ulysses4ever Jul 24, 2024

jasagredo Jul 24, 2024

geekosaur Jul 24, 2024

jasagredo Jul 24, 2024

geekosaur Jul 24, 2024

jasagredo Jul 26, 2024

ulysses4ever Jul 26, 2024

jasagredo Jul 26, 2024

geekosaur Jul 26, 2024

ulysses4ever Jul 27, 2024

jasagredo commented Jul 25, 2024

jasagredo commented Jul 26, 2024

jasagredo commented Jul 26, 2024

jasagredo commented Jul 29, 2024

ulysses4ever commented Jul 29, 2024

ulysses4ever commented Jul 30, 2024

ulysses4ever Jul 30, 2024 •

edited

Loading

jasagredo Jul 30, 2024

ulysses4ever Jul 30, 2024

ulysses4ever Jul 30, 2024

jasagredo commented Aug 28, 2024

geekosaur commented Aug 28, 2024

Various cabal-testsuite improvements #10225

Various cabal-testsuite improvements #10225

Conversation

jasagredo commented Jul 23, 2024 • edited by geekosaur Loading

jasagredo commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasagredo commented Jul 25, 2024

jasagredo commented Jul 26, 2024

jasagredo commented Jul 26, 2024

jasagredo commented Jul 29, 2024

ulysses4ever commented Jul 29, 2024

ulysses4ever commented Jul 30, 2024

ulysses4ever Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasagredo commented Aug 28, 2024

geekosaur commented Aug 28, 2024

Various `cabal-testsuite` improvements #10225

Various `cabal-testsuite` improvements #10225

jasagredo commented Jul 23, 2024 •

edited by geekosaur

Loading

ulysses4ever Jul 30, 2024 •

edited

Loading