Print args as sexp for consistency

[wasamasa]

I'm currently figuring out a failing test using :to-have-been-called-with and for some reason it prints only one side of the args as sexp and the other as string. This makes comparing the expected with the actual hard for no reason.

[snogge]

This looks good, but I think we need to go further.

On the second string, the "but it was called with..." should also use %S instead of manually unpacking its arguments.

I would also really like a test that shows the difference between before and after this change. I was surprised that the tests passed with this change. It probably shows that the tests in this area are not that good.

If you do not feel like making these further changes I'll try to get around to them soon-ish.

[wasamasa]

I wasn't sure what the deal with using commas to separate argument with was, so I left that alone. For easy diffing it might make more sense to just use the same print style for both sides. Or maybe even better, take a hint from ERT and show a diff with the offending difference that makes the test fail, using pretty-printing, maybe even a minimized s-expression still failing the test.

Testing the format of errors could be brittle and less of a benefit than expected. If you still want it, I'd consider making it fuzzier so that small adjustments of the text parts don't fail the test, but changes in the output of data in the error messages do.

[snogge]

I'm not sure what the comma separated format is about either, it predates my involvement in the project. I agree with the rest of your points, especially about the diffing of the arguments. That always makes hunting down problems easier.

[wasamasa]

I've looked into your suggestions again and found why the code doesn't just print the calls with %S: This case compares the expected arguments of a single call (that didn't happen) with all calls so far. If there is no match between the expected call in the list of all calls that happened so far, then all these calls need to be printed to give you a clue which one it is you were actually looking for. You could print it as a list of calls, but that wouldn't really make it less confusing. Example:

(defun my-test-function (arg) arg)

(describe "my-test-function"
  (before-each
    (spy-on 'my-test-function))
  (it "returns its arg unchanged"
    (my-test-function 1)
    (my-test-function 2)
    (my-test-function 3)
    (expect 'my-test-function :to-have-been-called-with 42)))

This will print:

FAILED: Expected `my-test-function' to have been called with (42), but it was called with (1), (2), (3)

42 is clearly a mistake here and it displays 1, 2 and 3 as possible alternatives you meant to use instead. It cannot know though, so diffing doesn't make sense here either, only for the case of a matcher that specifies two arguments that must be structurally equal. I could alternatively imagine making the message more verbose like:

FAILED: Expected `my-test-function' to have been called as (my-test-function 42)
EXPECTED: (my-test-function 42)
ACTUAL:   (my-test-function 1)
ACTUAL:   (my-test-function 2)
ACTUAL:   (my-test-function 3)

[snogge]

OK, that makes sense. That would mean that your change is relevant without any further change then?

[wasamasa]

Correct. The only thing I might add is a test case showing the difference between printing with %s and %S.

[snogge]

I updated the commit message slightly. Thank you for your contribution.