search

pyrmont / testament

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

Allow redefining tests with a dyn flag #9

Closed goto-engineering closed 3 years ago

goto-engineering commented 3 years ago

This allows me to re-evaluate test files in the REPL without running into the same-name constraint.

pyrmont commented 3 years ago

@goto-engineering Sorry it's taken so long to get to this and thanks for submitting the PR!

@subsetpark This looks good to me but do you have thoughts? The alternative would be to allow tests to be redefined. I'm not sure why I was so concerned that this might happen when I originally coded this up but arguably changing it now would be a breaking change (although in a somewhat technical sense since it'd be weird to have code that 'relied' on this behaviour).

subsetpark commented 3 years ago

I’d personally feel comfortable with simply warning on test redefinition, rather than disallowing it entirely.

it slightly off - maybe a little too low-level, or “out of band” - to control this behavior with a flag.

and I understand why redefining tests would be a behavior to watch out for; pretty much every time in my career that I’ve redefined a test, it’s been because i copy and pasted the form and forgot to change the name.

That said, I think printing a warning to the console “you are redefining test foo-bar on line baz; are you sure you want to do that?” Would totally accomplish the intention of alerting to this possibility, while still allowing someone in a repl context to perform the action.

pyrmont commented 3 years ago

Now that you say it, I remember that it was because of copying and pasting. I did that and it bit me so I decided to throw an error if it happened.

What do you think of changing the behaviour to output a warning rather than an error, @goto-engineering? After thinking it over, I feel similarly to @subsetpark that it's a little clunky to have a flag that you have to know to use if you're programming at the REPL.

goto-engineering commented 3 years ago

@pyrmont I'm ok with the warning; but note there are some other changes for REPL-mode.

  1. The test runners doesn't exit on failing test when in REPL mode, as this would quit the REPL
  2. The test stats are reset between runs in REPL mode, otherwise they keep stacking up and the numbers increase every time you run another test in the same test process.
subsetpark commented 3 years ago

Is there any reason for (2) not to obtain in all cases? In CLI mode, if the suite runs only once, it wouldn’t hurt to, for instance, ensure they’re reset at the beginning of the run.

goto-engineering commented 3 years ago

I don't know. I guess in regular mode it never needed to reset because the process would end after each run. @pyrmont ?

pyrmont commented 3 years ago

@goto-engineering Sorry it's taken a bit of time to get to this. Let me know what you think.

@subsetpark I updated register-test so that if you use a test name for multiple tests, it prints a warning to :err rather than throws an error. After thinking over the other changes, those all seem necessary for a good REPL experience so I've left them as they were. Do you have any thoughts?

goto-engineering commented 3 years ago

@pyrmont Looks good to me! :)

pyrmont commented 3 years ago

@goto-engineering Sorry this has been outstanding for so long >_<

You'll see I made a few tidying up changes, most notably changing the name of the dynamic variable to :testament-repl?. If you're happy with these, I'll merge into master.

pyrmont commented 3 years ago

@goto-engineering Thank you for contributing to Testament :) Sorry again for how long this took!

goto-engineering commented 3 years ago

Thanks for making Testament :)