Merge #2338

2338: Mark TRANS_TTL_{01,02} and STAKE_POOLS_JOIN_05 pending r=jonathanknowles a=Anviking

# Issue Number

None. Addressing CI failures.

# Overview


- [x] Add new `flakyBecauseOf ticketOrReason` helper that calls `pendingWith` unless `RUN_FLAKY_TESTS` is set.
- [x] Mark TRANS_TTL_{01,02} and STAKE_POOLS_JOIN_05 pending/flaky.
- [x] Add manual test calling for running flaky tests

# Comments

- Should lower the failure rate by 21% of runs, from 59% to 38%. 
- Next candidate for marking pending would be #2224, but with a relatively low failure rate of 3.6%, and being important, I think it would be a bad idea.
- Maybe we should have flaky tests run per default, unless setting `DONT_RUN_FLAKY_TESTS` in CI, to maximise the times we run them locally.

Recent bors failures:
```
succeded: 19, failed: 37 (66%), total: 56
excluding #expected failures

Broken down by tags/issues:
10 times #2292 Flaky test - various DB properties causing timeout | #2292
7 times #2295 Flaky TRANS_TTL_{01,02} - SlotNo 80 > SlotNo 50 | #2295
6 times
3 times #2311 Flaky test - integration test timeout after/related to STAKE_POOLS_LIST_01 | #2311
3 times #2230 Flaky  STAKE_POOLS_JOIN_05 - Can join when stake key already exists | #2230
2 times #2224 Flaky STAKE_POOLS_LIST_01 - List stake pools, has non-zero saturation & stake | #2224
1 times #another-integration-timeout  |
1 times #2337 STAKE_POOLS_GARBAGE_COLLECTION_01 timed out | #2337
1 times #2320 Flaky test - The node backend is unreachable at the moment. STAKE_POOLS_QUIT_02 | #2320
1 times #2295, #2331 Flaky TRANS_TTL_{01,02} - SlotNo 80 > SlotNo 50 | #2295
1 times #2207 Flaky SHELLEY_MIGRATE_01_big_wallet | #2207
1 times #2118 Property `prop_rebalanceSelection` occasionally fails. | #2118
```


<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/StakePools.hs

@@ -85,7 +85,7 @@ import Test.Hspec
 import Test.Hspec.Expectations.Lifted
     ( shouldBe, shouldSatisfy )
 import Test.Hspec.Extra
-    ( it )
+    ( flakyBecauseOf, it )
 import Test.Integration.Framework.Context
     ( Context (..), PoolGarbageCollectionEvent (..) )
 import Test.Integration.Framework.DSL
@@ -581,6 +581,7 @@ spec = describe "SHELLEY_STAKE_POOLS" $ do
 
     it "STAKE_POOLS_JOIN_05 - \
         \Can join when stake key already exists" $ \ctx -> runResourceT $ do
+        liftIO $ flakyBecauseOf "#2230"
         let walletWithPreRegKey =
                 [ "over", "decorate", "flock", "badge", "beauty"
                 , "stamp" , "chest", "owner", "excess", "omit"
@@ -1033,6 +1034,7 @@ spec = describe "SHELLEY_STAKE_POOLS" $ do
     it "STAKE_POOLS_GARBAGE_COLLECTION_01 - \
         \retired pools are garbage collected on schedule and not before" $
         \ctx -> runResourceT $ do
+            liftIO $ flakyBecauseOf "#2337"
 
             -- The retirement epoch of the only test pool that is configured
             -- to retire within the lifetime of an integration test run.

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Transactions.hs

@@ -88,11 +88,11 @@ import Network.HTTP.Types.Method
 import Numeric.Natural
     ( Natural )
 import Test.Hspec
-    ( SpecWith, describe, pendingWith )
+    ( SpecWith, describe )
 import Test.Hspec.Expectations.Lifted
     ( expectationFailure, shouldBe, shouldNotBe, shouldSatisfy )
 import Test.Hspec.Extra
-    ( it )
+    ( flakyBecauseOf, it )
 import Test.Integration.Framework.DSL
     ( Context
     , Headers (..)
@@ -607,6 +607,7 @@ spec = describe "SHELLEY_TRANSACTIONS" $ do
     let slotDiff a b = if a > b then a - b else b - a
 
     it "TRANS_TTL_01 - Pending transaction expiry" $ \ctx -> runResourceT $ do
+        liftIO $ flakyBecauseOf "#2295"
         (wa, wb) <- (,) <$> fixtureWallet ctx <*> emptyWallet ctx
         let amt = minUTxOValue :: Natural
 
@@ -640,6 +641,7 @@ spec = describe "SHELLEY_TRANSACTIONS" $ do
             & counterexample ("actual expiry: " <> show txActualExp)
 
     it "TRANS_TTL_02 - Custom transaction expiry" $ \ctx -> runResourceT $ do
+        liftIO $ flakyBecauseOf "#2295"
         (wa, wb) <- (,) <$> fixtureWallet ctx <*> emptyWallet ctx
         let amt = minUTxOValue :: Natural
         let testTTL = 42 :: NominalDiffTime
@@ -674,7 +676,7 @@ spec = describe "SHELLEY_TRANSACTIONS" $ do
             & counterexample ("actual expiry: " <> show txActualExp)
 
     it "TRANS_TTL_03 - Expired transactions" $ \ctx -> runResourceT $ do
-        liftIO $ pendingWith "#1840 this is flaky -- need a better approach"
+        liftIO $ flakyBecauseOf "#1840 -- need a better approach"
 
         (wa, wb) <- (,) <$> fixtureWallet ctx <*> emptyWallet ctx
         let amt = minUTxOValue :: Natural
@@ -2219,7 +2221,7 @@ spec = describe "SHELLEY_TRANSACTIONS" $ do
         txDeleteFromDifferentWalletTest emptyRandomWallet "byron-wallets"
 
     it "TRANS_TTL_DELETE_01 - Shelley: can remove expired tx" $ \ctx -> runResourceT $ do
-        liftIO $ pendingWith "#1840 this is flaky -- need a better approach"
+        liftIO $ flakyBecauseOf "#1840 -- need a better approach"
         (wa, wb) <- (,) <$> fixtureWallet ctx <*> emptyWallet ctx
         let amt = minUTxOValue :: Natural
 

lib/test-utils/src/Test/Hspec/Extra.hs

@@ -14,6 +14,7 @@ module Test.Hspec.Extra
     ( aroundAll
     , it
     , itWithCustomTimeout
+    , flakyBecauseOf
     ) where
 
 import Prelude
@@ -26,15 +27,19 @@ import Control.Concurrent.MVar
     ( MVar, newEmptyMVar, putMVar, takeMVar )
 import Control.Exception
     ( SomeException, catch, throwIO )
+import System.Environment
+    ( lookupEnv )
 import Test.Hspec
     ( ActionWith
+    , Expectation
     , HasCallStack
     , Spec
     , SpecWith
     , afterAll
     , beforeAll
     , beforeWith
     , expectationFailure
+    , pendingWith
     , specify
     )
 
@@ -143,3 +148,11 @@ await = takeMVar
 -- | Some helper to help readability on the thread synchronization above.
 unlock  :: MVar () -> IO ()
 unlock = flip putMVar ()
+
+-- | Mark a test pending because of flakiness, with given reason. Unless the
+-- RUN_FLAKY_TESTS environment variable is set.
+flakyBecauseOf :: String -> Expectation
+flakyBecauseOf ticketOrReason =
+    lookupEnv "RUN_FLAKY_TESTS" >>= \case
+        Just _ -> return ()
+        Nothing -> pendingWith $ "Flaky: " <> ticketOrReason

test/manual/cardano-node/FlakyTests.md

@@ -0,0 +1,15 @@
+# Run Flaky Tests
+
+Some tests might have been disabled in CI with the `flakyBecauseOf` helper.
+
+Run them locally using 
+
+```bash
+RUN_FLAKY_TESTS=1 stack test cardano-wallet:integration
+```
+
+or on Windows:
+```bash
+set RUN_FLAKY_TESTS=1
+cardano-wallet-test-integration.exe
+```