search

pyrmont / testament

A testing library for Janet
MIT License
28 stars 8 forks source link

Add defsuite! #8

Closed yumaikas closed 3 years ago

yumaikas commented 3 years ago

defsuite is a macro that automatically calls run-tests! after executing the body passed to it. It is intended to make (run-tests!) harder to forget, as I've forgotten it twice already. I prefer having something I write up front (in the pattern of defer), rather than something I have to remember at the end.

yumaikas commented 3 years ago

@pyrmont Is this something that you're interested in, or should I close out this PR?

pyrmont commented 3 years ago

@yumaikas I'm interested. I think you've identified a real problem but I also want to properly consider changes to Testament's API.

yumaikas commented 3 years ago

Ok, cool. I could turn this into an issue, then. I am using this in some of my current tests, so I'll be hanging on to my branch for the time being.

pyrmont commented 3 years ago

It's up to you but I have no problem with it being left as an outstanding PR if that's easiest.

yumaikas commented 3 years ago

@pyrmont I'm ok leaving it as an outstanding PR.

Which then means we can go into consideration:

What do you think is missing from this PR? Should the name be different? Should it take some of the arguments that run-tests! does? I gather that something about it doesn't quite sit well in your mind, I'd like to have an idea as to what.

pyrmont commented 3 years ago

Here are the questions I'm thinking about at the moment:

  1. I like what defsuite! looks like but it feels incorrect to me that a def-something would cause something like run-tests! to run. Wouldn't it make more sense to call it something like run-suite!? Except what is the 'suite' that it would run? None has been defined at the point it's called. Maybe it should be run!? Or test!? Or exercise!?

  2. In a similar vein, it's called defsuite but it doesn't actually creating a binding. Should it create some kind of binding? But what would be the purpose of such binding if it did create one? If it did create a binding, should there also be a run-suite! function?

  3. run-tests! calls os/exit as a way to stop jpm if there are failing tests (because otherwise jpm test will print out All tests passed. even if there are no passing tests). You can pass run-tests! the :exit-on-fail parameter to stop it from exiting but if you pass this parameter, run-tests! doesn't actually tell you whether the tests were passed or not. To be clear, this is an existing problem with Testament not your PR but perhaps it makes sense to fix that and return some value to indicate what tests passed/failed (perhaps including a data structure?) and have defsuite! proxy the parameters to, and return value from, run-tests!.

yumaikas commented 3 years ago

So, I think that do-tests might be a more accurate name. The ! on the end of the name was supposed to imply that something else was going on.

I think one thing that's going on is that right now, janet has a default assumption that tests will run automatically, where Testament assumes that the user will explicitly run tests.

I think, perhaps, that what might make more sense in the long run is to go from deftest to a test! macro that checks to see if it is part of a defsuite. If it isn't then it runs normally, if it is, then defsuite registers it to it's suite. Once a suite is done, it might call run-tests.... That's one thing that's kinda strange about testament's assertations: They are assertiony outside of a deftest

Thinking on it a bit, the following examples come to mind:

Tests that just run, without a suite surrounding them

  (def mytime (os/mktime { :year 2008 :month 0 :month-day 21 }))
  (def weekday (dt/weekday-str mytime))

  (test! weekday-str 
    (assert-equal "tuesday" weekday))

Tests that are part of a suite"

(suite! [date-and-time-tests :exit-on-fail true] 
  (def mytime (os/mktime { :year 2008 :month 0 :month-day 21 }))
  (def weekday (dt/weekday-str mytime))

  (test! weekday-str 
    (assert-equal "tuesday" weekday)))

Then, defsuite might delay tests until run-tests is called.

pyrmont commented 3 years ago

I liked your do-tests name so I went with that. My idea for the API was that it should be possible to pass a bracketed tuple (e.g. [:silent true]) as the first argument to do-tests and have it use that as the arguments for run-tests!. That's what this does at the moment. What do you think?

yumaikas commented 3 years ago

Looks good to me! :shipit:

pyrmont commented 3 years ago

I spent some more time thinking about do-tests and I decided in the end that I'd prefer a different name (I've gone with exercise!).

The new exercise! does three things:

  1. Consistent syntax: The name do-tests suggested that the macro would operate in the same way as do but this wasn't actually the case. Since we need a way to pass arguments to run-tests!, the previous version allowed a user to pass arguments using a bracketed tuple as the first argument. This syntax suggested that the macro would operate in the same way as let but that wasn't actually the case either. The new exercise! uses consistent syntax. The first argument must be a tuple and the remainder of the forms are evaluated in a do-form. That tuple can be empty if no arguments are to be passed.

  2. Use ! as a warning: After further reflection, I think the macro should end in !. This was not clear in the documentation but one of the reasons why it's run-tests! (rather than run-tests) is that the function will call os/exit by default and this will cause a Janet process to immediately terminate. Since the exercise! macro has the same side effect, it should be named similarly. (I have updated the docstring for run-tests! to make this behaviour clearer.)

  3. Cleaning up: Since the impetus for this PR was for a way to perform the testing in 'one hit', as it were, I think the macro should also call reset-tests! at the end. The macro now does that.

I appreciate your patience. I realise this is taking some time but I want to make sure I'm comfortable with new syntax before deciding how to add it. Do you have any additional thoughts?

yumaikas commented 3 years ago

What does reset-tests! do? If I exercise! multiple times in a single file, will that cause problems? (I do that for defsuite! in some of my current testatment-based suites, I'm curious as to what the change will be there).

yumaikas commented 3 years ago

So, after adding that to my local defsuite!, it looks like not having that as part of exercise! would be a bug, so definitely keep it added.

Once you get this merged, I'll move my suites using defsuite! over to exercise! and report back if anything important got missed. It's a simple enough underlying structure, however, I don't anticipate much difficulty, now that the names are in a better place.

pyrmont commented 3 years ago

Testament uses a number of top-level variables to track the performance of the test suite:

https://github.com/pyrmont/testament/blob/80949e87114ea62a0d9c5d02c96d5cbc7a869522/src/testament.janet#L9-L18

Assuming you're using the code you originally submitted in the PR, I would have expected multiple calls to defsuite! to produce incorrect test output as each call to defsuite! will cause things like num-tests-run to keep incrementing.

So, after adding that to my local defsuite!, it looks like not having that as part of exercise! would be a bug, so definitely keep it added.

I gather that's what you were noticing?

yumaikas commented 3 years ago

Yes, that is what I was noticing.

How would these reporting vars work if testing was to get multithreaded? I think Janet would do the "right" thing, and each thread would have it's own copy of the globals?

pyrmont commented 3 years ago

That's my understanding, too, and seems to be how things work based on some quick tests. Fortunately, that's all implementation so if it turns out to be different, I can look at going with a more thread-safe alternative.