Skip to content

Remove test.cb() #2387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
novemberborn opened this issue Feb 1, 2020 · 6 comments
Closed

Remove test.cb() #2387

novemberborn opened this issue Feb 1, 2020 · 6 comments
Assignees
Labels
breaking requires a SemVer major release scope:test-interface
Milestone

Comments

@novemberborn
Copy link
Member

test.cb() is a legacy API. It's probably time to remove it.

Ideally we can define an experiment, say "noCb" so users can opt in to not having test.cb() available.

When enabled, it should be removed from the chain:

https://github.com/avajs/ava/blob/56338dcb77246b5eca3c01e24d1cb6d66da8f71f/lib/create-chain.js#L77:L79

https://github.com/avajs/ava/blob/56338dcb77246b5eca3c01e24d1cb6d66da8f71f/lib/create-chain.js#L88:L92

Then by the time we're shipping the next breaking version we can remove it properly, from the TypeScript definition, the chain code and the implementation.

@sindresorhus
Copy link
Member

Before it’s removed, it’s important that we have docs in place that show common test.cb use-cases with test instead. For example, promisify, events (using p-event), streams (using async iterator or get-stream, etc.

@ash0080
Copy link

ash0080 commented Mar 25, 2020

Wish to include p-event similar internal,
events test is the shortcoming of AVA

@novemberborn
Copy link
Member Author

@ash0080 we'd rather people use p-event directly at this stage than add more API surface to AVA.

@novemberborn novemberborn self-assigned this Mar 29, 2020
novemberborn added a commit that referenced this issue Mar 29, 2020
With this commit, test.cb() has been removed. See #2387
novemberborn added a commit that referenced this issue Mar 30, 2020
With this commit, test.cb() has been removed. See #2387
novemberborn added a commit that referenced this issue Apr 4, 2020
With this commit, test.cb() has been removed. See #2387
novemberborn added a commit that referenced this issue Apr 5, 2020
With this commit, test.cb() has been removed. See #2387
novemberborn added a commit that referenced this issue Apr 12, 2020
With this commit, test.cb() has been removed. See #2387
novemberborn added a commit that referenced this issue Apr 13, 2020
With this commit, test.cb() has been removed. See #2387
@alastairs
Copy link

I'm writing tests for a Pulumi application, and based on their own appears those require the callback approach. Here's a (TypeScript) sample macro I've written using the callback API:

const hasTag: CbMacro<[AzureResource, string[]], PulumiContext> = (t, resource: AzureResource, expectedTags: string[]) => {
  pulumi.all([resource.name, resource.urn, resource.tags])
    .apply(([name, urn, tags]) => {
      t.deepEqual(Object.getOwnPropertyNames(tags), expectedTags,
        `Some required tags were missing from resource ${name} (${urn})`);

      t.end();
    });
}

I'd like to move away from the callback model, but It's not immediately obvious to me how to promisify this: what are the resolve and reject scenarios? Annoyingly, while Pulumi's API is asynchronous, it is only Promise-like and doesn't actually return Promises anywhere; hence the t.end(); is required inside the lambda function to indicate the test completion.

@novemberborn
Copy link
Member Author

@alastairs looks like const [name, urn tags] = await new Promise(resolve => pulumi.all([…]).apply(resolve)) to me.

@alastairs
Copy link

@novemberborn That worked a treat, thank you so much 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking requires a SemVer major release scope:test-interface
Projects
None yet
Development

No branches or pull requests

4 participants