approvals / ApprovalTests.cpp

Native ApprovalTests for C++ on Linux, Mac and Windows
https://approvaltestscpp.readthedocs.io/en/latest/
Apache License 2.0
318 stars 51 forks source link

Approvals::verify fails my test with an appended '\n' #50

Closed curtnichols closed 3 years ago

curtnichols commented 4 years ago
    void Write( std::ostream &out ) const
    {
        out << s << "\n";
    }

This causes my call to verify to fail. So I wonder:

Should Write be modifying my content?

claremacrae commented 4 years ago

@curtnichols

That's a very fair question. I don't think we considered people who might have pre-created approval files, and for whom the extra character written out might be a problem.

If we were to change the behaviour of StringWriter, we would break a whole load of existing tests, so this will need some thought.

An idea for a temporary workaround: we recently made it quite easy to supply custom writer implementation, so perhaps you could try supplying your own writer class that didn't add the extra newline?

There is documentation here: https://github.com/approvals/ApprovalTests.cpp/blob/master/doc/Writers.md#top

curtnichols commented 4 years ago

I might try that.

Also, I need to look into writing the "received" file myself, as I'm generating it with an external program that can easily save to file itself. I suspect that might look like a custom Writer that can format the raw data.

claremacrae commented 4 years ago

I'm not sure about this, but if all your tests are using pre-created data and calling external programs, it's starting to sound to me like maybe the Python implementation of Approval Tests might give you what you need - https://github.com/approvals/ApprovalTests.Python

Assuming you stick with the C++ version, there's one potential pitfall with the naming of the pure virtual methods in ApprovalWriter that we have not documented: https://github.com/approvals/ApprovalTests.cpp/blob/master/ApprovalTests/core/ApprovalWriter.h

The cleanUpReceived() method might sound like it is allowed to tweak the contents of the received file, but you should do any customisation of writing in write() - and in fact cleanUpReceived() is called to delete the received file on disk, if the test succeeded.

claremacrae commented 4 years ago

Oh - one more thought - is verifyExistingFile() any use to you: https://github.com/approvals/ApprovalTests.cpp/blob/01278b90684aedeac5a8bfa9d8941280297cc8d9/ApprovalTests/Approvals.h#L143 It is for when some other process already wrote out the received file, and so Approval Tests does not need to write anything...

curtnichols commented 4 years ago

verifyExistingFile sent me down a weird path, with an empty .approved.1 file being generated. Creating a custom writer is working nicely.

For context, the code under test is being executed from within the approval tests, the external program is just for converting binary image files into something diff-able.

claremacrae commented 4 years ago

The Reporter implementations that launch a diffing tool have to create an approved file, if it doesn’t yet exist, so that the diffing tool will launch and allow received to be copied to approved... Otherwise, most diff tools would just refuse to launch, saying one file did not exist.

So this is expected behaviour for most cases when first running a new Approvals test.

Glad you got the custom writer working!

Do you think this ticket needs any work now?

(I’ll probably leave it open anyway, as a reminder to document the things we talked about)

curtnichols commented 4 years ago

Thanks, I don't think I need anything more out of this issue.