Strapping down of Extend-routines in util.js #3442
Conversation
This is the next escalation on the war against the Travis unit tests failing (which came into being by yours truly). By accident, I could recreate the unit test failure on my development machine. This led to a more directed effort to squash the bug. The insight here is that test `(window === undefined)` fails, but `(typeof window === 'undefined`)` succeeds. This undoubtedly has to do with the special status `window` has as a global object. Changes: - Added check on presence of `window` in `Canvas._requestNextFrame()`, fixed local source errors. - Added catch clause in `CanvasRendered._determinePixelRatio()` - small fix: raised timeout for the network `worldCup2014` unit test
- Small fixes and cleanup in `util.js` - Removed `util.protoExtend()`, not used anywhere
wimrijnders
changed the title
| if (e.message() === 'window is undefined') { | ||
| console.warning("'" + e.message() + "' happened again"); | ||
| var msg = e.message; | ||
| //console.warn("Got exception with message: '" + msg + "'"); |
There was a problem hiding this comment.
Ah, yes, that one. I hope the travis unit test thing is truly dead now.
There was a problem hiding this comment.
I'm getting rid of the catch-block, it's not required any more.
| checkExtended(a, b); | ||
|
|
||
| // NOTE: if allowDeletion === false, null values are copied over! | ||
| // This is due to existing logic; it might not be the intention and hence a bug |
There was a problem hiding this comment.
Umm, I don't think it's a good behavior. It does seem like a bug that should be addressed. But on the other-hand, I usually take the approach that if a field is undefined, it should not be assigned a value of undefined whilst null is a legit value in a field and should be passed on in the extend. If that's the behavior of the function, I'm ok with it.
There was a problem hiding this comment.
I agree with you; for me the trade-off is between fixing it and keeping it like this so as not to break things elsewhere. I'll have a look if I can get rid of it, or confirm that the current working is good.
|
@yotamberk Please check Caveat comment in first post? There is stuff I encountered during making unit test of which I'm not sure to fix. It's almost legitimate but not consistent. |
* The next fix on Travis unit test failure This is the next escalation on the war against the Travis unit tests failing (which came into being by yours truly). By accident, I could recreate the unit test failure on my development machine. This led to a more directed effort to squash the bug. The insight here is that test `(window === undefined)` fails, but `(typeof window === 'undefined`)` succeeds. This undoubtedly has to do with the special status `window` has as a global object. Changes: - Added check on presence of `window` in `Canvas._requestNextFrame()`, fixed local source errors. - Added catch clause in `CanvasRendered._determinePixelRatio()` - small fix: raised timeout for the network `worldCup2014` unit test * Preliminary refactoring in utils.js * Added unit tests for extend routines, commenting and small fixes * More unit tests for extend routines * - Completed unit tests for extend routines in - Small fixes and cleanup in `util.js` - Removed `util.protoExtend()`, not used anywhere * Added unit tests for known font options
This is internal work, prompted by the investigation in #3408. Submitted here separately in order to keep the PR size(s) down. The reason to do this was to better understand the code, this PR records the results of the investigation for posterity.
Changes:
Some refactoring and fixes of the routines in
util.jsthat are used for copying data between objectsAdded (header) commenting for these routines
Added unit tests for these routines.
Extra: added unit test for invariability of font properties, because changing these has big consequences for the code in which they are used (paranoia by admission)
Caveat: There still is functionality present which might be considered buggy. Please scan in
test/util.test.jsfor the word 'bug' and give me your opinion on these. Is it something that should be fixed?