testing improvements

[turadg]

Description

#5766 ran into a problem with test.todo not being available in ses-ava. 524e7de adds it. (fyi @erights )

I also got fed up with this noise in test output. 295c8aa removes it.

Security Considerations

none

Documentation Considerations

--

Testing Considerations

Better

[kriskowal]

Are we confident that our emulation satisfies the entire ava interface now? Or, is this a convenient fiction that improves the debug experience?

[turadg]

You're right we don't. That's why this still returns TestInterface. But the avaTest param is indeed from Ava.

Marking this as Draft because my refactor broke some case.

[turadg]

Confident now that it satisfies because it passes through whatever it doesn't override. I typed the param as generic so that it wraps whatever version of Ava was passed in.

[turadg]

@kriskowal why do we have a whitelist of what to allow through? shouldn't it be an "override" list that's passed over after copying everything? IIUC "ses-ava" package is to provide SES-happy console output, not for actual security of the test run.

[kriskowal]

@kriskowal why do we have a whitelist of what to allow through? shouldn't it be an "override" list that's passed over after copying everything? IIUC "ses-ava" package is to provide SES-happy console output, not for actual security of the test run.

I should in this case defer to @erights, but this may have been born of abundance-of-caution or force-of-security-habit and eligible for a rethink.

[turadg]

I looked into the open issues for ses-ava and these would be solved by having the "override" list instead of an "allow" list:
endojs/endo#645
endojs/endo#647

But I don't understand why this fence was put here! @erights ?

I also don't know the symptoms of not using ses-ava at all. Do they persist in Ava 4?

[turadg]

Also, since the prepare-test-env is mutating the env, why not mutate Ava test itself? Status quo is we have these prepare-test-env-ava.js files that have both side-effects AND exports. Because they have side-effects they have to go before other imports. But then when they use relative paths our lint rules complain that it should be lower, so we have to suppress:

/* eslint-disable import/order */
import { test } from '../tools/prepare-test-env-ava.js';

There are 36 of those.

How about this instead?

import '../tools/prepare-test-env-ava.js';

import test from 'ava';

[erights]

Since I see @kriskowal already approved, I'm changing my comments to Request changes as an interlock, even though it is still not R4R

[erights]

@kriskowal why do we have a whitelist of what to allow through? shouldn't it be an "override" list that's passed over after copying everything? IIUC "ses-ava" package is to provide SES-happy console output, not for actual security of the test run.

Yeah, it isn't for security. It is to keep track of which methods we've examined to determine whether to augment them or not. Perhaps we should have two whitelists: The ones we know we want to augment, and the ones we know we don't. It is an interesting policy choice what to do with the remainder: Do we preserve their existing behavior (as your code does) until we decide, or do we omit them until we decide?

[erights]

Also, since the prepare-test-env is mutating the env, why not mutate Ava test itself? Status quo is we have these prepare-test-env-ava.js files that have both side-effects AND exports. Because they have side-effects they have to go before other imports. But then when they use relative paths our lint rules complain that it should be lower, so we have to suppress:

/* eslint-disable import/order */
import { test } from '../tools/prepare-test-env-ava.js';

There are 36 of those.

How about this instead?

import '../tools/prepare-test-env-ava.js';

import test from 'ava';

Whether or not we mutate test in place, shouldn't we still prefer the former to the latter because it is shorter (less big of a deal) and bundles related issues together so it is less of a refactoring hazard?

Also, wouldn't the first line of the latter still need to come first? If it does and still uses the '../tools/...' form you show above, wouldn't it still need the eslint-disable?

[erights]

I also don't know the symptoms of not using ses-ava at all. Do they persist in Ava 4?

The consequence is that the seeming console output is neither from the real console nor from the replacement that lockdown installs instead. Thus it lacks the crucial diagnostic info that's especially important when we're iterating the development of our test.

[turadg]

Yeah, it isn't for security.

Ok good. So the purpose of the "ses-ava" package isn't for security, just to accommodate SES's console handling when testing with Ava.

It is to keep track of which methods we've examined to determine whether to augment them or not. Perhaps we should have two whitelists: The ones we know we want to augment, and the ones we know we don't. It is an interesting policy choice what to do with the remainder: Do we preserve their existing behavior (as your code does) until we decide, or do we omit them until we decide?

I can't think of any reason to omit any part of the Ava test framework. The user shouldn't have to lose functionality because of SES's console requirements. (The lack of .todo was a surprise which broke my build and my focus.) For runtime, one list suffices: modify these methods so work better in SES. The rest should pass through. If there is any value in keeping track of "we looked into this one and it doesn't need augmentation", that's a documentation concern. A list like that in a comment above the override list would be fine by me, though I don't see the value in that. I expect whenever there's a problem that it'll get reported and then belong on the override list. If something is known to need overriding and that's not working yet, that could go into the override list, commented out with an explanation (and ideally a link to a ticket).

[turadg]

/* eslint-disable import/order */
import { test } from '../tools/prepare-test-env-ava.js';

How about this instead?

import '../tools/prepare-test-env-ava.js';

import test from 'ava';

Whether or not we mutate test in place, shouldn't we still prefer the former to the latter because it is shorter (less big of a deal) and bundles related issues together so it is less of a refactoring hazard?

I think those are both offset by the value of consistent Ava tests, regardless of whether SES env is prepared. What's "related" is "prepare env" and when you prepare env or not, it should not change the ergonomics of writing a test. By bundling shims which have side effects with exports of symbols to use, I argue that this is more of a refactoring hazard.

Also, wouldn't the first line of the latter still need to come first? If it does and still uses the '../tools/...' form you show above, wouldn't it still need the eslint-disable?

Good question. The way the rule works, if you import without a symbol it's assumed to have side effects so it's exempt from ordering rules.

Note that this PR isn't requesting the change. I asked to understand why it's like this in the first place and to see if we can agree on the proposed way. (I've since seen in PR history of Endo that it used to be the way I'm proposing.) If we do, I'll file a ticket to do that after MN-1.

But my immediate goal with this PR is to make SES-Ava conform to the Ava API.

[erights]

If there is any value in keeping track of "we looked into this one and it doesn't need augmentation", that's a documentation concern.

I am ok with this, and with your other points above that I did not comment on.

[erights]

Approving in a rush as explained at #5774 (comment)