Skip to content
This repository has been archived by the owner on Feb 13, 2024. It is now read-only.

#287 Dhaden/flush return rejectable promise #288

Closed

Conversation

Dahaden
Copy link
Contributor

@Dahaden Dahaden commented Aug 2, 2021

Referencing #287

  • Return rejected promises via flush
  • Attempted to fix tests (not quite there)

import test from 'ava'
import Analytics from '.'
import { version } from './package'
const { spy, stub } = require('sinon')
Copy link
Contributor Author

@Dahaden Dahaden Aug 2, 2021

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.')
Copy link
Contributor Author

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())
Copy link
Contributor Author

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)
Copy link
Contributor Author

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

@nd4p90x nd4p90x added this to In progress in analytics-node Aug 18, 2021
@Dahaden
Copy link
Contributor Author

Dahaden commented Sep 3, 2021

Closing this PR in favour of #294

@Dahaden Dahaden closed this Sep 3, 2021
analytics-node automation moved this from In progress to Done Sep 3, 2021
@pbassut
Copy link
Contributor

pbassut commented Sep 30, 2021

@Dahaden But this PR included some useful upgrades(like ava and babel), right?
I don't think we should ditch that

@Dahaden
Copy link
Contributor Author

Dahaden commented Sep 30, 2021

I can create another PR with just the ava bump if you want? The big changes are just:

  1. No import (because babel got removed from ava, might be a better way to do that though?)
  2. Change in API for Async functions,
  3. Change in API for error matching

@pbassut
Copy link
Contributor

pbassut commented Oct 1, 2021

That would be terrific @Dahaden .
We're on a really old version of Ava.

@pbassut
Copy link
Contributor

pbassut commented Oct 1, 2021

I like the baby steps approach here. Let's get this #288 going as it is the biggest chunk of the whole deal.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Promise returned by flush never rejects (and possible tests that do not work properly)
2 participants