Be able to run TUI applications with "stack test"

[thomasjm]

Hi @mpilgrem -- I have something I'm starting to feel good about for #5897!

This PR shows my latest and best approach. There is no longer a command-line flag; instead we just test whether stack test is being run inside a terminal using the existing hIsTerminalDeviceOrMinTTY function. If so, then we launch the test process using a simple proc function rather than the traditional ways which captures stdout/stderr; see here.

• Any changes that could be relevant to users have been recorded in ChangeLog.md.
• The documentation has been updated, if necessary

I've tested this by installing it on my machine and running (in the sandwich repo):

stack test demo-stack-test --test-arguments --tui

[mpilgrem]

@thomasjm, many thanks!

One thing on my mind is: in introducing this new functionality, is the existing functionality affected in any way? I don't have much time to work through that myself at present. Is that something you could address expressly in your testing, and document here? (I see the CI has green ticks, which is a start.)

On the linting fail, HLint's suggestion (in the CI logs) seems to me to be a good one. Please can you adopt it.

[thomasjm]

is the existing functionality affected in any way?

That's the question... the new behavior is only used

a) when run in a terminal, and
b) in the code path in the singleTest function (due to a commit I just added)

I think that limits the potential regressions reasonably well--normal builds and things being run in CI shouldn't be affected, for example.

I just ran some tests with both versions and compared them side-by-side and they look identical to me. I guess some questions remain:

• Make sure things still look good in interleaved mode?
• It looks like TTY mode could happen in in a ConcurrencyDisallowed action, i.e. benchmarks. Should make sure that still works.
• Does anything weird happen on Windows? -- I tried basic tests and it seems fine.

[mpilgrem]

@thomasjm, I am still working through this. If I understand correctly, OTConsoleTTY is like the old OTConsole Nothing except its use indicates that we know we are in a terminal.

I am not sure you need, or want, hIsTerminalDeviceOrMinTTY - I think the globalTerminal field of the GlobalOpts of the Runner of the Config of the BuildConfig of the EnvConfig already holds that information.

Again, if I understand correctly, the main 'point of departure' is that this assumes that if we are in a terminal, the user wants, simply:

runProcess ::  MonadIO m => ProcessConfig stdin stdout stderr -> m ExitCode

and not the existing manipulation of the ProcessConfig:

\pc0 -> do
  stdinBS <-
    if isTestTypeLib
      then do
        logPath <- buildLogPath package (Just stestName)
        ensureDir (parent logPath)
        pure $ BL.fromStrict
             $ encodeUtf8 $ fromString $
             show (logPath, mkUnqualComponentName (T.unpack testName))
      else pure mempty
  let pc = setStdin (byteStringInput stdinBS)
         $ setStdout output
         $ setStderr output
           pc0
  withProcessWait pc $ \p -> do
    case (getStdout p, getStderr p) of
      (Nothing, Nothing) -> pure ()
      (Just x, Just y) -> concurrently_ x y
      (x, y) -> assert False $ concurrently_ (fromMaybe (pure ()) x) (fromMaybe (pure ()) y)
    waitExitCode p

If that is correct, is the major difference the treatment of the standard input?

Seeking to understand the existing if isTestTypeLib took me to this documentation of the Cabal library: https://hackage.haskell.org/package/Cabal-syntax-3.8.1.0/docs/Distribution-Types-TestSuiteInterface.html#v:TestSuiteLibV09.

I understand that isTestTypeLib is examining the Cabal test suite interface. If the test-suite takes the form of an executable, according to the Cabal documentation, there should be nothing on standard input and Stack has, accordingly, setStdin (byteStringInput mempty). Does that mean, in a Cabal sense, your test executables, the ones that do require standard input, are 'non-conformant'?

If that is the case, it might be better to have an express flag - --[no-]allow-stdin, or something like that. That is stack test --allow-stdin.

I am wondering if the heart of this is as follows: if you are in a terminal, you are not isTestTypeLib, and you really want to --allow-stdin, you just want to skip the pc = setStdin (byteStringInput stdinBS) pc0?

[mpilgrem]

By way of exploring my idea above, I took the current Stack master branch code and simply removed the setting of the standard input:

let pc = setStdout output
       $ setStderr output
       pc0

I'm testing with a simple adaptation of the test executable in Spec.hs that stack new creates:

main :: IO ()
main = do
  putStrLn "Input a line"
  line <- getLine
  putStrLn $ "Test suite not yet implemented: " <> line

and, with the simple removal, stack test worked as you might hope.

So, I am wondering if the solution is just a simple flag added to stack build that allows standard input, when it is wanted.

[mpilgrem]

@thomasjm, I've implemented my idea at #5916 (comment) above in the fix5897 branch of this repository, see commit 99cb0f9. I have tested it on Ubuntu (WSL) with your Sandwich example: stack test --allow-stdin demo-stack-test --test-arguments --tui, and it seems to work.

The key part of my code is this extract, especially the line if toAllowStdin topts && isTerminal then pure id ...:

mec <- withWorkingDir (toFilePath pkgDir) $
  optionalTimeout $ proc (toFilePath exePath) args $ \pc0 -> do
    changeStdin <-
      if isTestTypeLib
        then do
          logPath <- buildLogPath package (Just stestName)
          ensureDir (parent logPath)
          pure $ setStdin
               $ byteStringInput
               $ BL.fromStrict
               $ encodeUtf8 $ fromString $
               show (logPath, mkUnqualComponentName (T.unpack testName))
        else do
          isTerminal <- view $ globalOptsL.to globalTerminal
          if toAllowStdin topts && isTerminal
            then pure id
            else pure $ setStdin $ byteStringInput mempty
    let pc = changeStdin
           $ setStdout output
           $ setStderr output
             pc0

(toAllowStdin has been added to TestOpts.)

What do you think? That seems to me more straight-forward than introducing additional data constructors of the OutputType type.

[thomasjm]

Hey, sorry for the delay, and thanks for looking into this!

It's a good catch that what we want to do can be enabled just by leaving stdin unchanged.

Also a good catch that Cabal already defines certain test suite interfaces, and that interactive TTY apps aren't conformant with the exitcode-stdio-1.0 interface.

My original plan was to open a PR against Cabal after the Stack one is done, since I'm primarily a Stack user. But your findings make me think I should have done Cabal first :). What should probably happen is I make a proposal for a new test suite interface for Cabal, called TestSuiteInteractiveV10 or something, which is similar to TestSuiteExeV10 but allows stdin.

However, who knows how long it will take for such a thing to land in Cabal, so it would be nice to land this in Stack in the meantime. The Cabal part might be an uphill struggle--for example, I notice that hpack doesn't even allow you to set the test suite type and forces it to default to exitcode-stdio-1.0.

Regarding your solution:

• I have no opinions on how it's implemented internally, but it would be nice if it didn't require an extra argument to opt in. Just thinking about the ergonomics of using it, it's kind of a lot to type if you have to do stack test --allow-stdin --test-arguments --tui. That's why my PR tries to auto-detect if you're in an interactive terminal so there's no extra arg.
• A naming nitpick: maybe a more specific name like --allow-test-stdin?
• If the long-term plan were to add a new test suite interface to Cabal, maybe it's a bad idea to add a flag like this to Stack that will just need to be deprecated later? That might be another reason to prefer an auto-detecting solution.

What do you think?

[thomasjm]

Opened an RFC on Cabal: haskell/cabal#8604

[mpilgrem]

@thomasjm, I have changed the name of the flag to --[no-]tests-allow-stdin and made it enabled by default. People who make test executables that follow the Cabal 'exitcode-stdio-1.0' specification should not notice that Stack is more lenient by default and, if they wish Stack to be strict, they can pass --no-tests-allow-stdin.

I have created a new pull request #5951 to give effect to the above.

[mpilgrem]

@thomasjm, having tested it, I think the current master branch version of Stack now works with the demo that you originally suggested. So I am going to close this pull request.