search

pyrmont / testament

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

run-tests! check for duplicate names? #1

Closed hinkelman closed 4 years ago

hinkelman commented 4 years ago

It would be nice of run-tests! checked if all the deftest names were unique. I tend to copy and paste deftest and might forget to the change the name. No big deal if all the tests pass, but could be confusing if more than deftest with the same name and I'm looking in the wrong place.

I should point out that I made the decision to just number the assertions using note in assert-equal so I'm throwing away the more detailed output that would negate the need to have run-tests! check for unique names. I don't have a good reason for doing that other than the output is a little cleaner.

pyrmont commented 4 years ago

@hinkelman I think the problem might be more with the way that register-test works:

https://github.com/pyrmont/testament/blob/c560aae1dc5aae94ac6298ca6362bbdd46a1c2ba/src/testament.janet#L71-L77

At the moment, it naïvely pushes each test onto an array. If it stored them in a table keyed by function name, it would mean that if you redefined a test, it merely overwrote the existing test.

A separate consideration is about whether you should need to provide a name at all (which sounds like what your problem is really about). Since most people presumably will run tests by calling run-tests! maybe it makes sense to have the name be optional and for me to just use an incrementing counter for 'anonymous' tests.

hinkelman commented 4 years ago

I definitely like the option to give tests a name because it provides some structure to my tests file. I certainly don't consider it a bug that it allows for duplicates names. testament is great as it is. No need to gum up the code to protect against this silly mistake. Thanks!

pyrmont commented 4 years ago

The reason to consider it a mistake is that deftest is modelled after clojure.test's deftest and intentionally defines a function. That 'function' can then be composed with other functions. I haven't updated Testament's test suite to cover this but if you define tests with the same name multiple times, I'm not sure what happens later when you try to refer to the test by its symbol. I presume it just uses the one that was defined last but that could lead to unexpected behaviour if you're doing dynamic things.

Are you saying that if I adopted the table approach to storing tests in the test suite that would break your workflow?

hinkelman commented 4 years ago

Oh, no, don't worry about my workflow. I just started exploring Janet. I have no workflow that can't be changed. I'm just saying that I don't have any understanding of how testament works under the hood so I wasn't sure what would be involved in adding a name check. I mostly asked in the spirit of thinking that if this change isn't too much work, then it might save me from myself in the future.

pyrmont commented 4 years ago

@hinkelman It took a while but I've decided to approach this by requiring tests to be uniquely named. Having tests have names makes the way results are logged more robust and allows the result data itself to include the name of the test (this is perhaps useful for users that want to use the new set-on-result-hook feature).

pyrmont commented 4 years ago

@hinkelman You mentioned you didn't really have a workflow at this point but for what it's worth, I've just added support for anonymous tests. You can now do this:

(deftest (is (= 1 2)))

and you'll get the following output:

> Failed: test_00000j
Assertion: (= 1 2)
Expect: 1
Actual: 2

(The test is named via a call to gensym so the actual name may differ.)

Not sure if that's of use to you, but if it is, that's great! :D