Add try-commit to test assertions #1947

qlonik · 2018-10-06T22:42:28Z

Hello. Some time ago there was a discussion in the #1692 issue. I tried to look around the code and see what could be done. I tried doing this but I'm not sure if it is the best solution.

This method creates new instance of Test class and runs the attempted assertion as part of that and then it updates main test if the attempted test passes or fails.
There were suggestions in the issue to create some Host class, which Test class will extend. Such Host class will only care about running assertions, while Test class will care about counting assertions and taking care of snapshots. I tried with Host class at first, however, I found that assertions are getting bound to the Test, and I was not sure which parts of Test need to be moved to Host and which can stay, without breaking assertions.
Also, creating new Host class, would require to re-implement the run() function and all of the edge cases, which are already figured out in the current implementation of Test().

There were also mentions about taking care of plan() assertion and about recursiveness of try() assertion. Also, snapshots were excluded from discussion, but I think they should be talked about too.
Since the current implementation of Test can handle all of these cases, it means reusing Test class as an instance for try() function should allow for those three things to work. There was also a comment about supporting the .discard() on promise returned from .try(), which is also added.

With the current implementation, each attempt has to be explicitly either accepted with .commit() or declined with .discard(). When the result is committed to, it increase assertion counter and copies the log of the attempt into main test. When the result is discarded, the assertion counter is not increased. It seems that it would be possible to remove explicit discard on the attempt result. However, in that case, there will be a requirement that user need to have at least one call to .commit() in order to pass the test.

I'm open to some suggestions about current implementation and to guidance how to make Host class work, if this is more desirable way.

Example usages with current implementation:

const twoRndInts = () => {
  const rnd = Math.round(Math.random() * 100);
  const x = rnd % 10;
  const y = Math.floor(rnd / 10);
  return [x, y];
};

test('test', async t => {

  const result = await t.try((t, a, b) => {
    t.is(a, b);
  }, ...twoRndInts());

  if (result.passed) {
    result.commit();
  } else {
    t.log(result.error);
    result.discard();

    const result1 = await t.try((t, a, b) => {
      t.is(a, b);
    }, ...twoRndInts());

    result1.commit();
  }
});

// EDIT: the following usage scenario with discarding incomplete attempt is not supported
test('test2', t => {
  const result = t.try((t, a, b) => {  // <-- result here is a pending promise
    return new Promise(res => setTimeout(res, 1000))
      .then(() => t.is(a, b));
  }, ...twoRndInts());

  return new Promise(res => setTimeout(res, 100))
    .then(() => {
      result.discard();
    });
});

TODO:

(Updated from #1947 (comment))

novemberborn · 2018-10-07T15:28:01Z

Oh wow, this is very exciting @qlonik!

I must admit I've kind of forgotten how Test and assertions work… It may take me a little while to review this and figure out what we need to do in order to land this PR. There's some other Babel related work that needs to take priority, we really need to get the 1.0 release out.

I'm terribly sorry about that. I'm rather geeked about shipping this feature!

lib/test.js

qlonik · 2018-10-07T19:25:33Z

how Test and assertions work

Yea, it was a bit difficult to follow. There are functions being bound to instance of Test all over the place.

I was thinking that it would be cool to refactor assertions as a class with whatever fields they require, and Test to extend that assertions class with snapshot management code. Additionally, ExecutionContext might be extending the Test class as well with log(), plan(), and try() assertions (or others as needed).

qlonik · 2018-12-15T21:21:47Z

I would like to rebase this branch on top of master. Anything I should worry about?

novemberborn · 2018-12-15T22:18:44Z

Go for it. I’ve been terribly busy of late, hoping to have a look after next week.

qlonik · 2018-12-15T22:57:23Z

Its okay if it takes time. Its holidays after the next week anyway, or something :)

I've been using snapshot of ava forced on commit that includes these changes. It worked for me quite well. It will be cool when it makes it into ava :)

novemberborn

Hey @qlonik sorry for the long wait. This looks pretty good! Some comments below.

index.d.ts

lib/test.js

novemberborn · 2019-01-12T15:19:47Z

Another thought. If we're limiting to one concurrent t.try() per test, we should explore reusing the t value inside the callback. That way you could write:

test('inline', async t => {
  const result = await t.try(() => {
    t.pass()
  })
  result.commit()
})

const reusable = t => t.pass()
test('reuse', async t => {
  const result = await t.try(reusable)
  result.commit()
})

We prefer the t variable name is used for the execution context, so it'd be confusing if an outer context is used in an inline t.try() callback.

qlonik

Some of the changes were made keeping in my use case of property testing. Here is basically how I'm using ava in the current iteration - https://gist.github.com/qlonik/9a297285284d71f7da47022f120ef4ad.

From this use case, there might be hundreds of t.try() runs inside the macro provided by the library. It is transparent to user. Whoever writes the test might need to debug the test using t.log, and so the macro needs to be able to print those logged values, so I thought that logs should be returned from the attempt. Similarly with the title of the attempt - it is an easy way to keep track of the current attempt to which logs belong. Maybe instead of the title, we could return the current attempt number from attempt counter?

I was wondering if we should explicitly fail when t.try() and commit()/discard() are not properly used. Should it be more explicit and throw errors when it is not properly used or should it be more implicit and allow some duplicate commit calls or discard after commit?

Do you have idea if there are some tests missing?

EDIT: Sorry for duplicate messages, I didnt know github would do that.

index.d.ts

lib/test.js

qlonik · 2019-01-13T17:35:46Z

lib/test.js

+				fn,
+				title: this.title + '.A' + (this.attemptCount++)
+			});
+			return new Test(opts);


I was adjusting the title in order to prevent any conflicts related to snapshots. Isn't it the case that if we adjust the title, then there are no problems about number of concurrent t.try() and both may contain snapshots?

qlonik · 2019-01-13T17:38:42Z

lib/test.js

@@ -213,6 +302,34 @@ class Test {
 		this.saveFirstError(error);
 	}

+	addPendingAttemptAssertion() {
+		if (this.finishing) {
+			this.saveFirstError(new Error('Assertion passed, but test has already finished'));


Should it be Attempt is complete, but the test has already finished?

qlonik · 2019-01-13T17:39:04Z

lib/test.js

+
+	countFinishedAttemptAssertion(includeCount) {
+		if (this.finishing) {
+			this.saveFirstError(new Error('Assertion passed, but test has already finished'));


Should it be the same as above?

lib/test.js

novemberborn

Whoever writes the test might need to debug the test using t.log, and so the macro needs to be able to print those logged values, so I thought that logs should be returned from the attempt.

Oh good point! Perhaps attempt.discard({ retainLogs: true }) would include them in the test results?

Similarly with the title of the attempt - it is an easy way to keep track of the current attempt to which logs belong. Maybe instead of the title, we could return the current attempt number from attempt counter?

Yea we should include that in the logs, but perhaps also have a form of t.try('unique attempt title', fn)?

I was wondering if we should explicitly fail when t.try() and commit()/discard() are not properly used. Should it be more explicit and throw errors when it is not properly used or should it be more implicit and allow some duplicate commit calls or discard after commit?

Commit-after-commit and discard-after-discard should be no-ops. They're harmless. Commit-after-discard and discard-after-commit should fail I think.

As per below, I think we should remove the discard-while-running behavior for now. What do you think about not allowing concurrent attempts?

index.d.ts

lib/test.js

novemberborn · 2019-01-20T17:08:54Z

lib/test.js

+				fn,
+				title: this.title + '.A' + (this.attemptCount++)
+			});
+			return new Test(opts);


The snapshots are indexed on the title, so if currently you only commit the third attempt, then later if the first attempt is committed you won't actually compare the snapshot. See also #1585.

novemberborn · 2019-01-20T17:09:43Z

lib/test.js

@@ -213,6 +302,34 @@ class Test {
 		this.saveFirstError(error);
 	}

+	addPendingAttemptAssertion() {
+		if (this.finishing) {
+			this.saveFirstError(new Error('Assertion passed, but test has already finished'));


Not sure at the moment, sorry 😄

novemberborn · 2019-01-20T17:09:47Z

lib/test.js

+
+	countFinishedAttemptAssertion(includeCount) {
+		if (this.finishing) {
+			this.saveFirstError(new Error('Assertion passed, but test has already finished'));


Not sure at the moment, sorry 😄

qlonik · 2019-01-20T21:22:21Z

With regards to concurrent attempts. I think it would be nice to allow attempt to be concurrent. From my use case, I'm passing async function to the library, and I have no control whether that is called concurrently or not, so I might have to do some magic outside of passed function to ensure that attempts run sequentially. It might affect performance in some cases.

Additionally, if we choose to do one test at a time or multiple tests at a time, switching to the other one will probably be a breaking change. Since people might rely on one or another behavior. Also, ava runs tests in parallel by default, so maybe attempts should also be run by default in parallel?

I would probably prefer concurrent attempts, unless we figure out that something breaks if it runs concurrently.

I'm still pretty confused with snapshots and test titles. It seems that if title of the attempt is changed, then the snapshot will be bound to that title. I'm not sure if it depends whether attempt is committed or discarded. The attempts should have the same title, unless the order of them is changed. Maybe this will be influenced by the fact there are concurrent attempts or not.

Last thing that I'm hesitant to change is about setting { failing: false, callback: false } vs setting some value to be { inline: true } and handling it specifically. There are various functions in various places that get bound to the instance of test, which I do not know about. I'm thinking that adding special behavior for { inline: true }, which will override some of the behaviors of failing and callback, might cause bugs where we wouldn't know or forget to change something related to failing/callback.

EDIT: It might be nicer to have special mode for attempts via { inline: true }, but I dont think I'm familiar enough with the code base to make that change.

novemberborn

Thanks for the fixes @qlonik. I think this is pretty close actually! Some comments below. Let me know if there's anything specific you'd prefer me to pick up.

I'm still pretty confused with snapshots and test titles. It seems that if title of the attempt is changed, then the snapshot will be bound to that title. I'm not sure if it depends whether attempt is committed or discarded. The attempts should have the same title, unless the order of them is changed. Maybe this will be influenced by the fact there are concurrent attempts or not.

Snapshots are indexed by test title, and then by order of invocation. E.g. if you have this test:

test('ordered snapshots', t => {
  t.snapshot(1)
  t.snapshot(2)
})

And then reorder the arguments, both assertions will fail.

Changing the test titles will definitely have unintended consequences. The problem with concurrent attempts is that each attempt should start at the same order number, else AVA won't be able to compare snapshots. We could make concurrent attempts work, but if multiple attempts used snapshots then we can't save both:

test('concurrent attempts', t => {
  const attempt = t2 => {
    t2.snapshot(true)
  }

  const [first, second] = await Promise.all([t.try(attempt), t.try(attempt)])
  first.commit()
  second.commit() // This must fail!
})

test('serial attempts', t => {
  const attempt = t2 => {
    t2.snapshot(true)
  }

  (await t.try(attempt)).commit()
  (await t.try(attempt)).commit() // This is fine
})

So yea, we can make concurrent attempts work! But we need some housekeeping for the snapshots to work correctly.

It might be nicer to have special mode for attempts via { inline: true }, but I dont think I'm familiar enough with the code base to make that change.

That's fine, I can pick that up.

index.d.ts

lib/test.js

index.d.ts

lib/test.js

index.d.ts

The commit() and discard() functions are split. Counting of attempt is increased by the increment value. Behavior of try, commit() and discard() are more implicit. Removes unneeded fields from commit().

…/ava into test-try-commit-assertion

novemberborn · 2019-08-18T14:05:54Z

About concurrent snapshots. The file with snapshots contains snapshots for assertion outside of attempts and assertions from within the first attempt, but not second. Maybe none of attempts should be recorded?

Snapshots from all committed attempts should be recorded. Are you describing a scenario in which this is not the case?

@qlonik do you mean that the snapshots from the first attempt are recorded, and then the test fails? I don't think that's any different from regular snapshots which are recorded before an assertion fails.

qlonik · 2019-08-18T14:13:31Z

Yes, I meant that. I think it's okay then.

…were any

…s finished

Though you may start multiple attempts.

novemberborn · 2019-08-18T14:36:43Z

@qlonik I've hopefully fixed the tests and made it so you can't run assertions outside of an active attempt. t.try() now counts as a single assertion as far as t.plan() is concerned. Do these changes look good to you?

I'm planning to make t.try() opt-in for now (see #2218) so we can land this at least, and then do follow-up work in master. Thank you so much for your work on this!

🎉

qlonik · 2019-08-18T15:09:36Z

The changes look good to me. The only thing I have to add and I'm not sure if it exists is the @experimental tag for jsdoc. If it does, we could add that tag into doc for those functions (but the word experimental in the beginning should still be kept, in case the ide doesn't support experimental tag)

novemberborn · 2019-09-08T15:33:08Z

The only thing I have to add and I'm not sure if it exists is the @experimental tag for jsdoc.

Oh I don't really know about JSDoc. But if it comes up we can update the definition for that. I'll prefix the comments with "Requires opt-in." though.

novemberborn · 2019-09-08T19:36:57Z

@qlonik I'll file the follow-up issues later, but for now 🎉 let's merge this! Thank you so much for your hard work on this, and for bearing with me over all these months.

qlonik · 2019-09-10T19:37:37Z

Yay 🎉! Thank you for helping with the feature too.

novemberborn · 2019-09-15T17:59:09Z

See https://github.com/orgs/avajs/projects/1 for the follow-up work. Let me know if I missed anything 😄

qlonik · 2019-09-15T18:05:18Z

I think the case when someone tries to use parent's assertions while inside the attempt is not addressed.

This is the only discussion we had about it so far:

Thinking about names for assertions in attempt made me realize that we do not handle the case when inside the attempt we are calling assertions from parent scope. So, maybe we should figure out what to do in that case, and if cannot how to avoid that, we could recommend to shadow the variable with parent assertions. Other than this issue, those names seem good.

Hmm. I suppose we could fail assertions on the parent context when attempts are pending, other than t.try() of course. Would be good to avoid that kind of confusion.

novemberborn · 2019-09-15T18:06:39Z

No I dealt with that: bb458be

qlonik · 2019-09-15T18:08:01Z

Ah, okay, I didn't notice. 👍

qlonik commented Oct 7, 2018

View reviewed changes

lib/test.js Outdated Show resolved Hide resolved

qlonik mentioned this pull request Jan 10, 2019

Conditionally perform assertions, allow tests to commit to the results #1692

Closed

novemberborn requested changes Jan 11, 2019

View reviewed changes

qlonik commented Jan 13, 2019

View reviewed changes

novemberborn reviewed Jan 20, 2019

View reviewed changes

novemberborn requested changes Jan 27, 2019

View reviewed changes

index.d.ts Outdated Show resolved Hide resolved

lib/test.js Outdated Show resolved Hide resolved

index.d.ts Outdated Show resolved Hide resolved

lib/test.js Outdated Show resolved Hide resolved

lib/test.js Outdated Show resolved Hide resolved

lib/test.js Outdated Show resolved Hide resolved

qlonik commented Jan 28, 2019

View reviewed changes

index.d.ts Outdated Show resolved Hide resolved

qlonik added 16 commits January 27, 2019 23:53

Add try-commit to test assertions

40fa08e

Don't use rest symbol to support Node 6

2521b31

Update TS typings with t.try()

8afddc2

Refactor tryTest: pull toCommit() defn from promise

d64a6fb

Make sure commit()/discard() only called once

fccf41b

No need to save return error of attempt twice

d1d559f

Use rest and spread on arguments to try function

6be281f

Count pending before executing passed fn

eeb861b

Make behaviour of try more implicit

7153337

The commit() and discard() functions are split. Counting of attempt is increased by the increment value. Behavior of try, commit() and discard() are more implicit. Removes unneeded fields from commit().

Return array of assertion errors from commit()

62ffd9b

Fix tests of the test class

ba690b7

Fix error message when test finished early

a4f0fee

Save logs from discard if needed

7750dfc

Remove discard before promise resolved

476148c

Move attempt counting into Test class

42a7245

Add optional title to attempt

e71ee0b

novemberborn mentioned this pull request Aug 18, 2019

Groundwork to support experimental features #2218

Merged

novemberborn added 2 commits August 18, 2019 15:50

Merge branch 'test-try-commit-assertion' of https://github.com/qlonik…

9503cb6

…/ava into test-try-commit-assertion

Merge branch 'master' into test-try-commit-assertion

1652c4e

novemberborn added 5 commits August 18, 2019 16:15

Report pending attempts or assertions before verifying whether there …

d26575e

…were any

Attempts count as a single assertion for parent tests

1920792

Fix error when an asynchronous assertion is started after the test ha…

16cc21e

…s finished

Assertions must not be run outside of an active attempt

bb458be

Though you may start multiple attempts.

Mark t.try() as experimental in the type definition

c302923

novemberborn mentioned this pull request Aug 21, 2019

How to get Assertion Error in after hook? #2219

Closed

Merge branch 'master' into test-try-commit-assertion

fa7812d

Require opt-in for t.try()

2b8ba3a

novemberborn merged commit 4fdb02d into avajs:master Sep 8, 2019

qlonik deleted the test-try-commit-assertion branch September 15, 2019 18:06

novemberborn mentioned this pull request Jan 5, 2020

Additional patterns so espower can be used with the new t.try() assertion avajs/babel#14

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add try-commit to test assertions #1947

Add try-commit to test assertions #1947

qlonik commented Oct 6, 2018 •

edited by novemberborn

Loading

novemberborn commented Oct 7, 2018

qlonik commented Oct 7, 2018

qlonik commented Dec 15, 2018

novemberborn commented Dec 15, 2018

qlonik commented Dec 15, 2018

novemberborn left a comment

novemberborn commented Jan 12, 2019

qlonik left a comment •

edited

Loading

qlonik Jan 13, 2019

qlonik Jan 13, 2019

qlonik Jan 13, 2019

novemberborn left a comment

novemberborn Jan 20, 2019

novemberborn Jan 20, 2019

novemberborn Jan 20, 2019

qlonik commented Jan 20, 2019 •

edited

Loading

novemberborn left a comment

novemberborn commented Aug 18, 2019

qlonik commented Aug 18, 2019

novemberborn commented Aug 18, 2019

qlonik commented Aug 18, 2019

novemberborn commented Sep 8, 2019

novemberborn commented Sep 8, 2019

qlonik commented Sep 10, 2019

novemberborn commented Sep 15, 2019

qlonik commented Sep 15, 2019

novemberborn commented Sep 15, 2019

qlonik commented Sep 15, 2019

Add try-commit to test assertions #1947

Add try-commit to test assertions #1947

Conversation

qlonik commented Oct 6, 2018 • edited by novemberborn Loading

TODO:

novemberborn commented Oct 7, 2018

qlonik commented Oct 7, 2018

qlonik commented Dec 15, 2018

novemberborn commented Dec 15, 2018

qlonik commented Dec 15, 2018

novemberborn left a comment

Choose a reason for hiding this comment

novemberborn commented Jan 12, 2019

qlonik left a comment • edited Loading

Choose a reason for hiding this comment

qlonik Jan 13, 2019

Choose a reason for hiding this comment

qlonik Jan 13, 2019

Choose a reason for hiding this comment

qlonik Jan 13, 2019

Choose a reason for hiding this comment

novemberborn left a comment

Choose a reason for hiding this comment

novemberborn Jan 20, 2019

Choose a reason for hiding this comment

novemberborn Jan 20, 2019

Choose a reason for hiding this comment

novemberborn Jan 20, 2019

Choose a reason for hiding this comment

qlonik commented Jan 20, 2019 • edited Loading

novemberborn left a comment

Choose a reason for hiding this comment

novemberborn commented Aug 18, 2019

qlonik commented Aug 18, 2019

novemberborn commented Aug 18, 2019

qlonik commented Aug 18, 2019

novemberborn commented Sep 8, 2019

novemberborn commented Sep 8, 2019

qlonik commented Sep 10, 2019

novemberborn commented Sep 15, 2019

qlonik commented Sep 15, 2019

novemberborn commented Sep 15, 2019

qlonik commented Sep 15, 2019

qlonik commented Oct 6, 2018 •

edited by novemberborn

Loading

qlonik left a comment •

edited

Loading

qlonik commented Jan 20, 2019 •

edited

Loading