pyrmont / testament

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

Adds streaming results and stack-frame info #3

Closed roobie closed 4 years ago

roobie commented 4 years ago

This is a work in progress, but I'm opening the PR so we can discuss. There are a couple of code comments, marked WIP: (shoulda called it RFC) about the changes, which I'll remove if and when this PR becomes mergeable.

The main points here are:

roobie commented 4 years ago

Maybe we could use https://github.com/andrewchambers/janet-where-defined instead of debug/stack?

pyrmont commented 4 years ago

@roobie My strong preference for Testament is that it have no dependencies so I'd prefer to not add anything unless absolutely required.

I like the hook implementation generally but am not sure about the stack frame stuff. I haven't worked in a language that gives you such easy access to debugging the stack and confess I don't really understand the advantages of exposing it. Is there a reason why Testament needs to return the frame? Isn't the stack visible to whatever function is running? If a custom reporter wants access to it, couldn't it handle that by itself?

roobie commented 4 years ago

You raise good points! I was probably locked in my train of thought that I wanted to read the stack at macro expansion, but now that you point it out, it's more likely over-engineered. I'll remove the stack frame stuff and clean it up.

roobie commented 4 years ago

Updated. I'm still interested in hearing your thoughts on the :do-exit part. I added that because I needed to run a bit of code after run-tests!.

pyrmont commented 4 years ago

@roobie I had badly chosen the names of the keys used in the report and results structs and confused myself so I went back and changed the keys to be :passed?, :details and :note. I think this is clearer.

In addition, since it's now important what the value of :details is in those structs, I went in and wrote detailed explanations in the different assertions. That means the user has to jump to the relevant assertion but hopefully that's not too difficult (especially if they're using the API documentation that will be generated by Documentarian—it will omit the docstrings for the private functions so there won't be as many functions to scan through as there are when reading the source code). This does mean that there's a risk that the docstring and the value of :details get out of sync but I don't see any way around that.

Finally, while I like the idea of providing the reports to a user-defined function, I am a teeny bit apprehensive that this reduces our flexibility to change the result struct in the future. To be sure, this is a self-imposed restraint. As a personal preference, I want to maintain backwards compatibility as much as possible and so once we expose it publicly, I don't want to make any changes to the result struct that isn't additive (for existing assertions). That would mean, for example, that we can't change the name of the :details key in the future.

Given that, what do you think about the result format?

roobie commented 4 years ago

Nice work on the documentation! 👍

You raise good points. A couple of things come to my mind with regards to this. For one, it could be decided that the result struct is internal to Testament and should not be exposed directly to user code, but instead we would define the minimal acceptable struct (and map/transform the result to it before passing it to the hook) for user code to make use of, like e.g. {:passed? <bool> :name <string> :description <string>} - i.e. just simple data, no (nested) data structures, and the minimal (I think) representation to be able to report something of use when a test is OK or fails. In this case :description could be the rendered version of :details or any string, really. The point is that the consumer shouldn't make assumptions about what's in the string. This would be a protocol, and as such, it could be versioned. So if a need to change this struct in a non-backwards-compatible manner, Testament would have to bump the major version number.

roobie commented 4 years ago

Also, I think the code changes look good. I'll be able to try them out later this evening.

pyrmont commented 4 years ago

@roobie Sorry to push again so soon but after reading your suggestion, I had the realisation that the result struct should really just be data. The form of the error message that Testament displays for failed assertions should be kept separately.

To that end, the result struct now has five keys: :kind, :passed?, :expect, :actual and :note. I think this gives hooks more flexibility to do what they want with the result. It also allowed me to centralise the documentation about what to expect in the set-on-result-hook docstring.

What do you think?

roobie commented 4 years ago

No need to apologize. I think your update makes sense, and as you say, it gives a lot more flexibility compared to earlier. I'd say that those five keys covers any reporting use-case I can think of. For example: let's say we would want to implement :deep-equal assertion support (i.e. deep=). Then any consumer relying on this hook need not do anything, because the data in the result that is passed to the hook is opaque in that way. Sure the actual data may differ, but the handling of it is the same.

I've created a gist, with a TAP printer using Testament from this branch: https://gist.github.com/roobie/b47b997bd1178ff4a50509efa174622e You can pipe the output from that into e.g. tap-parser and it'll correctly parse it and give reasonable diagnostic output as well (specifically the actual vs. expected now possible with your changes)

pyrmont commented 4 years ago

Awesome :) I think I'll merge it in, then. Thanks for all the help!

pyrmont commented 4 years ago

@roobie It occurred to me as I was writing the commit message that each assertion should really know what test it is associated with (this could then be part of the reported result). I know I wanted to do this originally but couldn't work out how to do it. I think I might now have an idea. The great thing is that result is just a map so if you build something depending on this version, it's no problem if I add that as an additional key 🎉