Fix broken unit tests #636
Conversation
| exports = module.exports = prepare; | ||
| // Add indirection to be able to spy on prepare | ||
| exports = module.exports = (...args) => exports.prepare(...args); | ||
| module.exports.prepare = prepare; |
There was a problem hiding this comment.
I am sorry to say that I really do not like this construct. It exports an API that looks like it may be consumed in multiple ways which I think we do not want to support in the future.
I would like to propose the following compromise solution: change exports.prepare to exports._prepare
There was a problem hiding this comment.
I do not like doing this for testability only, but I've given this a lot of thought and this seems to be the best solution for function-exports. Look at it as an alias. It has the added benefit of allowing you to do
const { prepare, preparePlatforms } = require('.../prepare');which I find quite nice.
There was a problem hiding this comment.
Hmm I did not see that case before, it does make more sense now. However that kind of aliasing continues to strike me as ugly both to understand and to support in an API. This is of course symptomatic of the quality of the existing design.
I am also not so happy that the commit message does not describe that this kind of aliasing is done and that the comment in the code also does not really describe the aliasing.
I would favor the following kind of approach:
- keep the aliasing, with descriptive comments in both the commit and in the code, along with a TODO comment to improve the design in this area
- raise an issue to rework the code and possibly the API to solve this in a better way
|
The reason that this has surfaced, seems to be jasmine/jasmine#1590. I'll pin Jasmine to 3.1 and re-evaluate this PR later. |
A change in Jasmine 3.2 seems to be responsible for the recent CI failures. Pinning it to 3.1 for now. See apache/cordova-ios#393, See jasmine/jasmine#1590 See #636
This fixes some unit tests that appear to have been broken by 60ba439 and then some that were uncovered during the fixing.