Basic macro support. #832

jamestalmage · 2016-05-14T09:24:30Z

This adds basic macro support as discussed in #695.

It does not completely implement the spec outlined there, specifically:

The macro can only specify the title using a function in macroFn.title. We discussed allowing macroFn.title to also be a string, and allowing some form of template language for extracting a title. However, using ES2015 string templates is already pretty easy, so we may just skip this.
~~We discussed allowing an array of macro functions.~~

Both the above proposals are found in this comment. They both enhance the implementation found in this commit, and would not break the contract. So I don't think there is anything preventing us from shipping this now.

mention-bot · 2016-05-14T09:24:31Z

By analyzing the blame information on this pull request, we identified @kasperlewau, @BarryThePenguin and @naptowncode to be potential reviewers

This adds basic macro support as discussed in avajs#695. It does not completely implement the spec outlined there, specifically: - The macro can only specify the title using a function in `macroFn.title`. We discussed allowing `macroFn.title` to also be a string, and allowing some form of template language for extracting a title. However, using ES2015 string templates is already pretty easy, so we may just skip this. - We discussed allowing groups of tests to be created using arrays. Both the above proposals are found in [this comment](avajs#695 (comment)). They both enhance the implementation found in this commit, and would not break the contract. So I don't think there is anything preventing us from shipping this now.

nfcampos · 2016-05-14T10:10:29Z

The same reason that makes it worthwhile to pass arguments as an array to the test function without spreading them should make us want to give them as an array to test(), or is it somehow more likely we'll want in the future to introduce a third agumment to the test function than a last argument to test()?

jfmengels · 2016-05-14T10:40:32Z

readme.md

+test('2 * 3 === 6', macro, '2 * 3', 6);
+```
+
+You can build the test title programatically by attaching a `title` function to the macro:


programmatically

jfmengels · 2016-05-14T10:55:17Z

🎉 Yay, this is great @jamestalmage! Didn't know whether this was accepted, but glad it's going forward, it's a really nice feature IMO.

jfmengels · 2016-05-14T10:56:17Z

readme.md

@@ -524,6 +524,33 @@ test.only.serial(...);

 This means you can temporarily add `.skip` or `.only` at the end of a test or hook definition without having to make any other changes.

+### Test macros
+
+Additional arguments after the test function will be passed as an array to the test function. This is useful for creating reusable test macros.


Wouldn't it rather be "creating reusable tests"?

That may be a cleaner way of saying it, but I'm trying to introduce the term. Maybe "reusable macros", but I think it's good as is.

novemberborn · 2016-05-15T14:33:25Z

LGTM!

jamestalmage · 2016-05-15T16:05:14Z

What about @nfcampos argument? Should it be arrays both places? Spread both places?

novemberborn · 2016-05-15T17:10:01Z

Spread in the implementation does prevent us from adding any additional arguments, but having a third argument wouldn't be very pretty anyway.

An array in the declaration is just annoying and prone to confusion. So I guess I'm leaning towards spread in both places.

jamestalmage · 2016-05-15T18:57:39Z

I'm leaning towards spread in both places.

Me too, but that pretty much closes that portion if the API from further expansion. That's a big decision, so let's wait for more input.

jfmengels · 2016-05-15T19:39:35Z

I would've preferred spread in both places, but it does get tricky if you want to add new args.

If we go for spread and want to go for arrays later, then I think that writing a codemod will be kind of tricky. (easy in most cases, but hard when the macro is defined in a separate file).

I'd go for consistency at least: either spread in both places or arrays in both places.

jamestalmage · 2016-05-15T23:26:48Z

Updated:

Switched to arg spreading everywhere.
Added array of macros support.

jamestalmage · 2016-05-15T23:30:23Z

Should the macro.title function get passed title as the first arg?

function macroA() {...}
function macroB() {...}

macro.title = (title, ...remainingArgs) => title + ' - macroA';
macro.title = (title, ...remainingArgs) => title + ' - macroB';

test('groupA', [macroA, macroB], 'some', 'args');
test('groupB', [macroA, macroB], 'more', 'args');

results in the following test titles:

groupA - macroA
groupA - macroB
groupB - macroA
groupB - macroB

novemberborn · 2016-05-16T10:16:22Z

Should the macro.title function get passed title as the first arg?

I think that'd be good, but it makes it harder to write macros since it'd be optional. I'm specifically thinking about adding a space separator:

macro.title = (prefix) => prefix + ' - macroA';

Let's assume prefix defaults to the empty string:

macro.title = (prefix) => (prefix ? prefix + ' - ' : '') + 'macroA';

Maybe if the resulting title is always trimmed on its left so you don't have to worry about the spacing?

novemberborn · 2016-05-16T10:16:27Z

LGTMO!

jfmengels · 2016-05-16T14:48:51Z

Should the macro.title function get passed title as the first arg?

👍

jamestalmage · 2016-05-16T16:52:41Z

I think defaulting to the empty string is a good plan. That's still falsy. We should trim results both left and right I say.

jamestalmage · 2016-05-16T17:03:24Z

For spacing, what if we allowed them to return an array of strings which we joined with a space character?

macro.title = (title) => [title, 'macroA'];

Or maybe we allow this:

macro.title = {prefix: 'macroA - '};
// or
macro.title = {suffix: ' - macroB'};

jfmengels · 2016-05-16T17:34:37Z

I think the former is a bit unnatural. I'd say stick to a function, and seek to improve later it if it feels insufficient.

jamestalmage · 2016-05-16T19:23:30Z

51bed36 passes title as first argument (or empty string if none provided).

I think this should be merge-able now.

@sindresorhus - Haven't heard much from you on this. Specifically, are you good with spreading the args?

sindresorhus · 2016-05-16T19:48:20Z

readme.md

+Additional arguments passed to the test declaration will be passed to the test implementation. This is useful for creating reusable test macros.
+
+```js
+function macro(t, input, expected) {


@jamestalmage Good thing we decided not to pass the context as the second argument 😝

Yeah, eating my words on that. Though not as badly as I've been forced to eat these ones.

Then why are you bringing up the Babel plugin? Is that something someone would use IRL?

sindresorhus · 2016-05-16T19:53:46Z

jfmengels · 2016-05-16T20:43:48Z

🎉 Folks, 0.15 is starting to get pretty exciting :D

jamestalmage force-pushed the macro-support branch from 453daa5 to f779999 Compare May 14, 2016 09:40

jfmengels reviewed May 14, 2016
View reviewed changes

jamestalmage added 3 commits May 15, 2016 17:13

fix readme indentation

8e6c294

spread arguments

a465fb4

Allow arrays of macros

06b8491

pass providedTitle as first argument to title function.

51bed36

sindresorhus reviewed May 16, 2016
View reviewed changes

improve docs

20e46f8

jamestalmage merged commit a454128 into avajs:master May 16, 2016

jamestalmage deleted the macro-support branch May 16, 2016 20:42

Ajinkya009 mentioned this pull request Mar 9, 2021

[Snyk] Security upgrade chokidar from 1.7.0 to 3.0.0 Ajinkya009/ava#7

Open

Ajinkya009 mentioned this pull request Nov 28, 2023

[Snyk] Fix for 3 vulnerabilities Ajinkya009/ava#15

Open

Ajinkya009 mentioned this pull request May 14, 2024

[Snyk] Security upgrade chokidar from 1.7.0 to 3.0.0 Ajinkya009/ava#16

Open

Basic macro support. #832

Basic macro support. #832

Uh oh!

Conversation

jamestalmage commented May 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented May 14, 2016

Uh oh!

nfcampos commented May 14, 2016

Uh oh!

jfmengels May 14, 2016

Choose a reason for hiding this comment

Uh oh!

jfmengels commented May 14, 2016

Uh oh!

jfmengels May 14, 2016

Choose a reason for hiding this comment

Uh oh!

jamestalmage May 15, 2016

Choose a reason for hiding this comment

Uh oh!

novemberborn commented May 15, 2016

Uh oh!

jamestalmage commented May 15, 2016

Uh oh!

novemberborn commented May 15, 2016

Uh oh!

jamestalmage commented May 15, 2016

Uh oh!

jfmengels commented May 15, 2016

Uh oh!

jamestalmage commented May 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamestalmage commented May 15, 2016

Uh oh!

novemberborn commented May 16, 2016

Uh oh!

novemberborn commented May 16, 2016

Uh oh!

jfmengels commented May 16, 2016

Uh oh!

jamestalmage commented May 16, 2016

Uh oh!

jamestalmage commented May 16, 2016

Uh oh!

jfmengels commented May 16, 2016

Uh oh!

jamestalmage commented May 16, 2016

Uh oh!

sindresorhus May 16, 2016

Choose a reason for hiding this comment

Uh oh!

jamestalmage May 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sindresorhus commented May 16, 2016

Uh oh!

jfmengels commented May 16, 2016

Uh oh!

Uh oh!

jamestalmage commented May 14, 2016 •

edited

Loading

jamestalmage commented May 15, 2016 •

edited

Loading

jamestalmage May 16, 2016 •

edited

Loading