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 a runTests function #168

Merged
merged 1 commit into from
Mar 19, 2019
Merged

Conversation

erikd
Copy link
Contributor

@erikd erikd commented Mar 11, 2018

And actually use it in the Hedhehog tests.

Closes: #97

@thumphries @jystic @nhibberd

Test.Hedgehog.Text.tests
main =
runTests
[ Test.Hedgehog.Text.tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably good to keep the same syntax style here, start of list on previous line that is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Fixed the formatting in the comment for runTests as well.

@thumphries
Copy link
Member

I feel a bit weird exporting a function that calls exitWith from the top level, especially with an innocuous name like runTests. Wondering if it would make more sense from Hedgehog.Main or something like that? Or a name with suite or main in it?

(not requesting changes per se, just wondering how you two feel about it)

@erikd
Copy link
Contributor Author

erikd commented Mar 11, 2018

I'm not married to the name, I just don't want to have to redefine this function in every test suite I write. I also think what you said makes a lot of sense. So how about:

  • We rename it runTestsuie.
  • Move it to Hedgehog.Main and not export if from Hegdehog requiring people to explicitly import Hedgehog.Main.
  • Since if calls exitFailure on any failure, maybe it should also call exitSuccess on success. Ie this function always terminates.

@erikd
Copy link
Contributor Author

erikd commented Mar 11, 2018

Hmm, now that I think about it, I'm mot so sure about the last part. I could imagine wanting to do:

   runTests
        [ Test.Wibble.One
        ]
  -- Only run the following tests if the previous one passed.
   runTests
        [ Test.Wibble.Two
        , Test.Wibble.Three
        ]

@moodmosaic
Copy link
Member

Move it to Hedgehog.Main and not export if from Hegdehog requiring people to explicitly import Hedgehog.Main.

Hmm... Isn't this a little bit weird? Perhaps it could be exposed as 'internal' from Hedgehog itself so we keep importing only the essential stuff:

import           Hedgehog
import qualified Hedgehog.Gen as Gen
import qualified Hedgehog.Range as Range

@erikd
Copy link
Contributor Author

erikd commented Mar 11, 2018

The idea of putting this in a separate Hedgehog.Main is growing on me. Maybe we should also provide more than one runner. Hedgehog.Mainmight provide fhe functions:

setLineBufferring :: IO ()

-- Run the tests and return `True` if they all passed, `False` otherwise.
runTestsuite :: [IO Bool] -> IO Bool

-- If any test fails, exit with `ExitFailure`.
runTestsuiteExitOnFail :: [IO Bool] -> IO ()

-- This _always_ exits, `ExitFailure` if any test fails, `ExitSuccess` otherwise.
runTestsuiteExit :: [IO Bool] -> IO a

@moodmosaic
Copy link
Member

Are we 100% sure about the Main part? Not that I can currently come up with an alternative...

@erikd
Copy link
Contributor Author

erikd commented Mar 11, 2018

@moodmosaic The import you have in your example are the ones you would put in an actual test suite. I usually keep actual tests out of the top level test runner. With this new functionality, my top level test runner would look like:

import qualified Hedgehog.Main as HMain

import qualified Test.Wibble.One
import qualified Test.Wibble.Two
import qualified Test.Wibble.Three

main :: IO ()
main = do
  HMain.setLineBuffering
  HMain.runTestsuiteExit
    [ Test.Wibble.One.tests
    , Test.Wibble.Two.tests
    , Test.Wibble.Three.tests
    ]

I'm not married to the name Main. Might be Run or Runner instead. Or maybe with these new names and type signatures we can just export them from Hedgehog.

Update: I think the new naming and type signatures are specific enough for these to be exported from Hedgehog.

Update 2: Exposing setLineBuffering is probably a waste. Its a cheap operation and these test runners should just set line buffering regardless.

@gwils
Copy link
Contributor

gwils commented Mar 11, 2018

I don't like the name Hedgehog.Main, but Run or Runner I could get behind. It just feels a bit wrong to me for a library (not an executable) to export a module called Main. I would prefer that this be exported from Hedgehog rather than a separate module regardless.

My preference is the version that calls exitFailure on failure but does not call exitSuccess on success. As Erik has pointed out, you may wish to call many test runs sequentially.

@erikd
Copy link
Contributor Author

erikd commented Mar 11, 2018

My preference is the version that calls exitFailure on failure but does not call exitSuccess on success.

This is why my (strawman) proposal now has three versions of the runner function :).

--
-- Given a number of separate modules all defining test funtion named `tests`,
-- the individual module level tests can be collected together and run in
-- sequence (one module of tests at a time) and the funcion will will exit
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: there seems to be an extra space between sequence.
Nitpick: will occurs two times in a row on that sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this PR will not go ahead as it is anyway.

@erikd erikd force-pushed the topic/test-runner branch 4 times, most recently from 86a8ac0 to e5f6fab Compare March 14, 2018 01:23
@thumphries
Copy link
Member

Closing this based on Slack discussions we had back in March.

The decision was to not rush into exporting a top-level runner, since public exports are forever and we all had some reservations about the current design. There is also the question of terminal encoding, Windows, etc to consider.

@thumphries thumphries closed this May 28, 2018
@moodmosaic moodmosaic mentioned this pull request Dec 10, 2018
@jacobstanley jacobstanley reopened this Mar 19, 2019
And actually use one in the Hedhehog tests.

Closes: hedgehogqa#97
@jacobstanley
Copy link
Member

💯

@jacobstanley jacobstanley merged commit fc98d47 into hedgehogqa:master Mar 19, 2019
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.

6 participants