CI is inconsistent #669
CI is inconsistent #669
Conversation
| expectLine :: (HasCallStack) => Handle -> (String -> Filter) -> Assertion | ||
| expectLine = expectLine' "" | ||
| expectLine :: | ||
| (HasCallStack) => Maybe (Int, String) -> Handle -> (String -> Filter) -> Assertion | ||
| expectLine Nothing h f = expectLine' "" h f | ||
| where | ||
| expectLine' s0 h f = do | ||
| line <- hGetLine h | ||
| expectLine' s0 h' f' = do | ||
| line <- hGetLine h' | ||
| let | ||
| trimmed = trim line | ||
| s1 = s0 <> "\n" <> line | ||
| cont = expectLine' s1 h f | ||
| cont = expectLine' s1 h' f' | ||
| if null trimmed | ||
| then cont | ||
| else case f trimmed of |
There was a problem hiding this comment.
1: We usually prefer to use foo0, f001, foo1 etc over foo, foo', foo'' because it tends to get difficult to read since you have to count the primes.
2: When you have a single worker function we usually call it go to prevent the need of using foo1 or foo'.
3: You can use eta reduction, you don't have to add h and f as arguments of expectLine. You simply return a partially applied go function.
| expectLine (Just (timeout, msg)) h f = do | ||
| initTime <- getCurrentTime | ||
| expectLine' initTime "" h f | ||
| where | ||
| timeout' = toRational timeout | ||
| expectLine' t0 s0 h' f' = do |
There was a problem hiding this comment.
| expectLine (Just (timeout, msg)) h f = do | |
| initTime <- getCurrentTime | |
| expectLine' initTime "" h f | |
| where | |
| timeout' = toRational timeout | |
| expectLine' t0 s0 h' f' = do | |
| expectLine (Just (timeout, msg)) = getCurrentTime =<< go | |
| initTime <- getCurrentTime | |
| expectLine' initTime "" h f | |
| where | |
| timeout' = toRational timeout | |
| go t0 s0 h f = do |
| if toRational (diffUTCTime t0 now) >= timeout' | ||
| then errorIO $ "expect line timed out on: `" <> msg <> "`. previously read: \n" <> s0 | ||
| else do | ||
| line <- hGetLine h | ||
| let | ||
| trimmed = trim line | ||
| s1 = s0 <> "\n" <> line | ||
| cont = expectLine' t0 s1 h' f' | ||
| if null trimmed | ||
| then cont | ||
| else case f trimmed of | ||
| Continue -> cont | ||
| Stop Ok -> pure () | ||
| Stop (Error msg') -> do | ||
| rest <- readRemainingChars h' | ||
| putStrLn $ "handle buffer dump:\n" <> rest | ||
| errorIO $ | ||
| "expect line failed on `" | ||
| <> msg | ||
| <> "`\nmessage: `" | ||
| <> msg' | ||
| <> "`\npreviously read:\n" | ||
| <> s1 |
There was a problem hiding this comment.
Would it make sense to create a custom return type to differentiate between Success, Timeout and Closed?
There was a problem hiding this comment.
That I'm not sure of - I'm not entirely certain whether that info is actually useful to the postproc functions since I haven't read over them properly, just updated the call locations for the functions I changed.
| line <- hGetLine h | ||
| let | ||
| trimmed = trim line | ||
| s1 = s0 <> "\n" <> line |
There was a problem hiding this comment.
I think I'd rather see a list of lines of type [String] rather than reading on a per line basis and then manually re-inserting new lines. You can easily convert [String] to String using unlines.
| Stop (Error msg') -> do | ||
| rest <- readRemainingChars h' | ||
| putStrLn $ "handle buffer dump:\n" <> rest | ||
| errorIO $ |
There was a problem hiding this comment.
You seem to trade assertFailure for errorIO, is there a reason for that?
| -- Create function to log the output of the processes | ||
| loggingSequence = do | ||
| threadDelay 1_000_000 -- Wait 1 second for data loggers to catch up | ||
| putStrLn "Picocom stdout" | ||
| picocomOut <- readRemainingChars picocomStdOut | ||
| putStrLn picocomOut | ||
| case maybePicocomStdErr of | ||
| Nothing -> pure () | ||
| Just h -> do | ||
| putStrLn "Picocom StdErr" | ||
| readRemainingChars h >>= putStrLn |
There was a problem hiding this comment.
I see you are removing picocom stderr printing from the logging infra, do you think we don't need it?
There was a problem hiding this comment.
Mostly just wasn't sure how to integrate it into the new stuff. I would probably have to include another argument to the function, something like onErr which would run in the error branches, but I'm not sure if that would be overcomplicating things.
rslawson
force-pushed
the
rs/ciDebugging
branch
from
2e46fc1
to
41fc738
Compare
rslawson
force-pushed
the
rs/ciDebugging
branch
16 times, most recently
from
157d532
to
c6604d4
Compare
rslawson
force-pushed
the
rs/ciDebugging
branch
from
c6604d4
to
b3b4c41
Compare
rslawson
force-pushed
the
rs/ciDebugging
branch
from
b3b4c41
to
72e11c0
Compare
CI seems to fail spuriously on timeouts, almost exclusively on the Vex RISC-V TCP test and in invoking Picocom. However, when certain functions time out, their previously read input is discarded, often along with what's left in the buffer being read from. These should instead probably be printed out on the program's stdout so that CI logs show what occurred prior to the timeout.