search

r-lib / R6

Encapsulated object-oriented programming for R
https://R6.r-lib.org
Other
403 stars 56 forks source link

check also error messages in tests #258

Closed IndrajeetPatil closed 2 years ago

IndrajeetPatil commented 2 years ago

Currently, tests are only testing if the errors occur, but additionally capturing the error message assures us that the error was produced for expected reasons, and not for some other, unexpected reasons.

IndrajeetPatil commented 2 years ago

@wch Let me know if you think testing this error snapshot only once is sufficient, since I am currently generating snapshots for all of them.

wch commented 2 years ago

@IndrajeetPatil I actually would prefer not to have all these errors snapshotted. I think the benefits are very, very minor, and it introduces more complexity in terms of files and stuff to deal with if, for example, the wording of an error message changes.

Now, I could imagine a case where there are two plausible error messages, one that you're actually looking for, and one that would be caused by a bug, but I think those cases are very rare -- and it would be clearer if the expectation is written directly in the test file instead of saved in a separate snapshot file. I've dealt with test snapshots in other projects where you have to keep bouncing back and forth between the test code and snapshot file in order to see what the actual vs. expected behavior is, so unless there's a large amount of text in the expectation, I think it's better to keep the expectation in the code.

IndrajeetPatil commented 2 years ago

Thanks for the detailed explanation about your reservations!

Having been bitten recently by the fragility of snapshot testing, I completely get your point. Makes sense to not introduce additional complexity for what might be extremely rare cases of multiple error messages.