-
Notifications
You must be signed in to change notification settings - Fork 150
#287 Dhaden/flush return rejectable promise #288
#287 Dhaden/flush return rejectable promise #288
Conversation
import test from 'ava' | ||
import Analytics from '.' | ||
import { version } from './package' | ||
const { spy, stub } = require('sinon') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annoyingly babel was removed from ava and imports arent native until node 13 so have to revert to requires here
@@ -88,7 +88,8 @@ test('expose a constructor', t => { | |||
}) | |||
|
|||
test('require a write key', t => { | |||
t.throws(() => new Analytics(), 'You must pass your Segment project\'s write key.') | |||
const error = t.throws(() => new Analytics(), { code: 'ERR_ASSERTION' }) | |||
t.is(error.message, 'You must pass your Segment project\'s write key.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With latest versions of ava, you can no longer provide just a string.
I can make this consistent with the other tests, or use this method for testing exceptions. Either or.
@@ -296,7 +297,7 @@ test('enqueue - skip when client is disabled', async t => { | |||
test('flush - don\'t fail when queue is empty', async t => { | |||
const client = createClient() | |||
|
|||
await t.notThrows(client.flush()) | |||
await t.notThrowsAsync(() => client.flush()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For anything that returns a promise, we have to use t.throwsAsync
or t.notThrowsAsync
now
try { | ||
await t.throwsAsync(() => client.flush()) | ||
} catch (error) { | ||
console.log('Dafuq:', error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of debugging in an attempt to find where the unhandled promise rejection was coming from.
This console log is not called at all. Very confused
Closing this PR in favour of #294 |
@Dahaden But this PR included some useful upgrades(like ava and babel), right? |
I can create another PR with just the ava bump if you want? The big changes are just:
|
That would be terrific @Dahaden . |
I like the baby steps approach here. Let's get this #288 going as it is the biggest chunk of the whole deal. |
Referencing #287