Let Jasmine handle async test results #619
Conversation
There was a lot of sub-optimal error handling in the tests. This commit has multiple benefits - Removes 700 LOC w/out removing any testing - Improves output when async tests fail - Should make async tests less error prone - Reduces Q-API usage to prepare for removal of Q (CB-14159)
| @@ -31,6 +31,8 @@ var TIMEOUT = 240 * 1000; | |||
| * should initially be in pkg.json and/or config.xml. | |||
| */ | |||
|
|
|||
| /* eslint "promise/catch-or-return": "error" */ | |||
There was a problem hiding this comment.
Why put eslint "promise/catch-or-return": "error" here? Shouldn't it be in a .eslint config file (doesn't have to be top-level)?
There was a problem hiding this comment.
That's an oversight. Thanks, will remove! I used that during the refactor to spot places where a Promise needed to be returned.
|
+1 from a quick scan 🎉 |
|
All right, gonna merge this... |
|
@brodybits I haven't actively looked into reasons for CI failures. But if anything, this should help make the tests more robust, yes. Having skimmed through almost all of the tests now, I have to say there's some horrible code in there and setup/teardown of test dirs is often pretty crude too. So I rather expect that there are still tests that are note stable. #613 will hopefully further improve the situation. |
Definitely. I've done that for one or two already (create/fetch I guess). Should be easier for everything other than lib. Maybe an issue with sub-tasks would be in order. However, I mainly started this to reduce Q-usage. Basically we need to remove usage of Q instance methods in depending packages to be able to return native Promises in the dependencies. Thus, |
That would definitely be good!
I am starting to agree with this. JIRA issues are really good for logging & tracking changes, but if I label PR discussions for multiple components under the same JIRA issue it becomes cluttered pretty quickly.
💯 I was already thinking to raise an issue to reduce or hopefully eliminate Q usage, guess I should leave it in your hands for now.
Yup, understood. I guess we are all in the same boat:) |
|
There was a lot of sub-optimal error handling in the tests.
Instead of handling Promise rejection and resolution ourselves, we now
just return them from the test functions and let Jasmine handle them.
Moreover, this also fixes tons of negative tests that looked something like this:
f().then(function () {
expect('foo').toBe('bar');
}).catch(function (err) {
expect('' + err).toMatch('some message');
});
Problems being:
- `expect('foo').toBe('bar')` is less obvious than `fail(...)`
- catches its own Error from the `.then` only to fail because `err` is missing
- Does not check that err is an Error and that the message is in `.message`
So these were normalized to look like this:
f().then(function () {
fail('Expected promise to be rejected');
}).catch(function (err) {
expect(err).toEqual(jasmine.any(Error));
expect(err.message).toMatch('some message');
});
All in all, this commit has the following benefits:
- Removes 700 LOC w/out removing any testing
- Improves output when async tests fail
- Should make async tests less error prone
- Reduces Q-API usage to prepare for removal of Q (CB-14159)
There was a lot of sub-optimal error handling in the tests.
Instead of treating every promise like the unique little sparkly star that it is, we just return them from the test functions and let Jasmine handle them.
This also fixes tons of negative tests that often looked something like this:
These were normalized to something like this (spot all the differences! 😉):
This commit has multiple benefits
I really don't expect anyone to review this ~2k LOC change set. Since it only affects the tests and they still pass I would merge this without a review if no one objects.