Assertion error messages handle symbolic method names #1642
Conversation
test/assert-test.js
Outdated
|
|
||
| if (typeof Symbol === "function") { | ||
| describe("with symbol method names", function () { | ||
| function setupSymbol(symbol) { |
There was a problem hiding this comment.
Could you please simplify the logic by making it more functional and more explicit? It seems needlessly complex right now 😃
- change the setup method
- remove the
bindcall in line 1662 - simplify
this.messagefunction (see below) and rename it - beforeEach => before
- improve test descriptions
test/assert-test.js
Outdated
| } | ||
|
|
||
| beforeEach(function () { | ||
| this.setupSymbol = setupSymbol.bind(this); |
test/assert-test.js
Outdated
| this.setupSymbol = setupSymbol.bind(this); | ||
|
|
||
| /*eslint consistent-return: "off"*/ | ||
| this.message = function (method) { |
There was a problem hiding this comment.
Simplify into
function createExceptionMessage(method, arg) {
try { sinonAssert[method](arg); }
catch (e) { return e.message; }
}All the uses have at most one argument so we can drop the slicing.
test/assert-test.js
Outdated
| }; | ||
| }); | ||
|
|
||
| it("assert.called exception message with symbol with description", function () { |
There was a problem hiding this comment.
The sentence is not valid English. Make it read out aload in a natural way, for instance it("should create an exception message for symbols that describe the symbol"
test/assert-test.js
Outdated
|
|
||
| it("assert.called exception message with symbol with description", function () { | ||
| var symbol = Symbol("Something Symbolic"); | ||
| this.setupSymbol(symbol); |
test/assert-test.js
Outdated
| var symbol = Symbol("Something Symbolic"); | ||
| this.setupSymbol(symbol); | ||
|
|
||
| assert.equals(this.message("called", this.obj[symbol]), |
There was a problem hiding this comment.
assert.equals(createExceptionMessage("called", obj[symbol]), ...
Resolves sinonjs#1640 (assertion failure errors with "Cannot convert a Symbol value to a string")
|
Cleaned up the tests. Please take another look. Thanks! |
|
Very nice, thanks! |
|
@fatso83 shall we cut a new release? |
|
yes, please :) |
|
This has been released with |
Resolves #1640 (assertion failure errors with "Cannot convert a Symbol value to a string")
Background (Problem in detail)
Assertion error messages may reference a method's name (which may now be a symbol, not a string). However, symbols cannot be implicitly converted to strings.
Solution
Explicitly cast symbols to strings with
String. The value returned should match http://www.ecma-international.org/ecma-262/6.0/#sec-symboldescriptivestring.How to verify - mandatory
npm installThe error message should say something about expecting
Symbol(my symbol)to be called, not an error about converting the symbol to a string.Checklist for author
npm run lintpasses