feat(test): Add "name", "origin" and "parent" to "Deno.TestContext" #14007

hyp3rflow · 2022-03-17T04:25:00Z

This PR includes features for implementing snapshot test feature (assertSnapshot) in deno_std.

Exposes the current test name.
TestContext includes their parentContext. (.parent)
Add origin (I think we can use this instead of import.meta.url or Deno.mainModule to get the path of snapshot).
~~Teardown API to call function after the test finished.~~
~~Add update flag on cli/test for snapshot update mode.~~

related comment is here: #3635 (comment)

I wrote simple test for this code, but I am not sure where I have to put this test file. please let me know where to put and how to run this test in the project structure. thanks!

Deno.test('test name', (t) => {
	t.name; // This will be `test name`
	t.origin; // `file:///Users/me/deno.test.ts`

	t.step('test name2', (t2) => {
		t2.name; // This will be `test name2`
		t2.origin; // `file:///Users/me/deno.test.ts`
	})
})

CLAassistant · 2022-03-17T04:25:05Z

All committers have signed the CLA.

dsherret

LGTM! Seems reasonable to add this name property

cli/dts/lib.deno.ns.d.ts

hyp3rflow · 2022-03-17T15:44:32Z

@dsherret I updated the PR but it was delayed due to github status issue. sorry 😢
If you think the PR has to be separated, feel free to comment me :)

cli/flags.rs

dsherret

The new name property seems nice, but I'm not sure this snapshotting feature should be in the CLI. It seems like something a testing framework (or deno_std) could implement themselves. cc @bartlomieju

cli/dts/lib.deno.ns.d.ts

hyp3rflow · 2022-03-17T16:28:49Z

And I forgot to push the Deno.test.teardown commit. 3f7d86c (#14007)
for the snapshot testing, I think there are two choices for writing the snapshot file.

Handle the writing section on 40_testing.js/runTests
Expose the new teardown API to register a function that will call after the testing for the module is finished.

I think the second one is better because handling snapshot testing in the core seems not ideal.
Is there any other solution for this problem? Here is the example using teardown API. denoland/std#2039

kitsonk · 2022-03-17T22:45:00Z

but I'm not sure this snapshotting feature should be in the CLI

I also agree that the snapshotting feature should not be part of CLI.

bartlomieju · 2022-03-17T23:08:26Z

but I'm not sure this snapshotting feature should be in the CLI

I also agree that the snapshotting feature should be part of CLI.

@kitsonk I'm confused. Are you for adding this functionality to CLI directly or against?

kitsonk · 2022-03-17T23:13:01Z

but I'm not sure this snapshotting feature should be in the CLI

I also agree that the snapshotting feature should be part of CLI.

@kitsonk I'm confused. Are you for adding this functionality to CLI directly or against?

Not. Sorry... edited previous comment.

hyp3rflow · 2022-03-18T01:31:26Z

Okay. I agree that snapshotting feature should not be part of CLI.
then is it okay to update the snapshot with the argument, not cli flags?
(like this, https://github.com/sramam/klick#filtered-snapshot-update)

deno test --allow-read --allow-write ./snapshot.test.ts -- --update

and check Deno.args in assertSnapshots

hyp3rflow · 2022-03-19T16:56:38Z

@bartlomieju Is there any way to cleanup after tests are done? I cannot find the way to writing snapshot after tests without the teardown API. Or do you think a new API like writeSnapshot on std side is needed?

bcheidemann · 2022-03-19T17:04:21Z

Deno.test.teardown(fn): to register function to be called after the test done

@hyp3rflow why do you need this API? It seems cleanup can be done without this API. It's been considered several times and we decided not to do it, so I'm quite hesitant ATM.

@bartlomieju

Hope you don't mind me responding to this @hyp3rflow - feel free to add to/correct my reply as necessary :D

As I understand it, the teardown function is introduced here for performance reasons. It is common to have many snapshots in a single test file and without the teardown function, it would be impossible to implement the assertSnapshot function without opening, reading, parsing and writing the file on every single call to assertSnapshot. This may be negligible on a small code base but in a mature code base with hundreds of snapshots this would have a significant performance impact.

To illustrate this, I implemented a basic test to compare the performance of using a teardown function to not using a teardown function. This is the code:

const NUMBER_OF_SNAPSHOTS = 100;

const obj: Record<string, Array<string>> = {};

for (let i = 0; i < NUMBER_OF_SNAPSHOTS; i++) {
  obj[`key${i+1}`] = new Array(100).fill(new Array(100).fill('a').join(''));
}


Deno.test(async function noTeardown() {
  await Deno.writeTextFile('test1.json', JSON.stringify(obj, null, 2));

  const start = Date.now();

  for (let i = 0; i < NUMBER_OF_SNAPSHOTS; i++) {
    const snapshots: Record<string, Array<string>> = JSON.parse(await Deno.readTextFile('test1.json'));
    snapshots[`key${i+1}`] = new Array(100).fill(new Array(100).fill('a').join(''));
    await Deno.writeTextFile('test1.json', JSON.stringify(snapshots, null, 2));
  }

  const stop = Date.now();

  await Deno.writeTextFile('result1.txt', `${stop-start}ms`);
});

Deno.test(async function withTeardown() {
  await Deno.writeTextFile('test2.json', JSON.stringify(obj, null, 2));

  const start = Date.now();

  const snapshots: Record<string, Array<string>> = JSON.parse(await Deno.readTextFile('test2.json'));
  for (let i = 0; i < NUMBER_OF_SNAPSHOTS; i++) {
    snapshots[`key${i+1}`] = new Array(100).fill(new Array(100).fill('a').join(''));
  }
  await Deno.writeTextFile('test2.json', JSON.stringify(snapshots, null, 2));

  const stop = Date.now();

  await Deno.writeTextFile('result2.txt', `${stop-start}ms`);
});

These are the results (AMD Ryzen 5 3600 and high-ish end SSD):

I put this together in a couple of minutes just to validate my intuition so let me know if you think this is not a valid test or if there's any mistakes!

A cleanup function (e.g. teardown) is also necessary to be able to detect and remove unused snapshots and to provide useful output after a test has finished running (e.g. no. snapshots updated).

Further to the above, I also feel that setup/teardown functions are a fairly standard and uncontroversial part of most testing frameworks (e.g. mocha, jest, jasmine, karma etc all offer before/after each/all functions). These could of course be implemented in a 3rd party testing library but this would probably need to duplicate a lot of the work already done as part of Denos test implementation in managing and running tests/test steps.

Could you elaborate on why setup/cleanup functions were rejected? I tired to search the GitHub issues but couldn't find the discussion! :P

hyp3rflow · 2022-03-19T17:27:09Z

@bcheidemann that's okay, I don't mind :) I'm thanks to you to reply elaborate comments related to this API.
I'm happy that we can build this PR together 😊

hyp3rflow · 2022-03-22T14:13:20Z

Well, I think we can use unload event instead of teardown API here. @bcheidemann

I pushed commit on assertSnapshot proposal: denoland/deno_std@418cc43 (#2039)
Using globalThis.onunload, we can implement assertSnapshot without implementing teardown API and performance issues. If you think this is okay, I'll remove the teardown implementation here and make an issue for it.

bartlomieju · 2022-03-22T17:13:31Z

Well, I think we can use unload event instead of teardown API here. @bcheidemann

I pushed commit on assertSnapshot proposal: denoland/deno_std@418cc43 (#2039) Using globalThis.onunload, we can implement assertSnapshot without implementing teardown API and performance issues. If you think this is okay, I'll remove the teardown implementation here and make an issue for it.

If you could get away with onunload that would be preferable than adding a new API 👍

dsherret

~~LGTM~~ approved the wrong PR

dsherret

Sorry, I approved the wrong PR when attempting to retry approving another PR due to the GH outage. I have yet to review this.

hyp3rflow · 2022-03-23T14:54:56Z

@dsherret okay, I will re-request your review :)

runtime/js/40_testing.js

dsherret

I think this seems more reasonable, but origin can already be retrieved via import.meta.url so I don't think we need it. We'll need to add tests for the name and parent functionality.

runtime/js/40_testing.js

cli/dts/lib.deno.ns.d.ts

hyp3rflow · 2022-03-25T03:58:45Z

@dsherret where can I write the test for name, origin, parentContext?
I found cli/tests/testdata/test/steps but I think this is for step on test cli not for testing how test context works.

dsherret · 2022-03-28T19:53:04Z

@hyp3rflow sorry for my delay. It can be added to cli/tests/unit/testing_tests.ts or some similar file. Basically, write some tests and then assert what's in the context.

hyp3rflow · 2022-03-28T20:44:45Z

@dsherret that's okay! I added some tests for testContext on testing_tests.ts.

dsherret

LGTM. Thanks @hyp3rflow!

dsherret · 2022-03-29T16:48:00Z

cli/dts/lib.deno.ns.d.ts

+    /**
+     * File Uri of the current test code.
+     */
+    origin: string;


Considering that import.meta.url is a string, I think it makes sense for this to be a string as well.

hyp3rflow · 2022-03-31T14:45:36Z

@kitsonk @bartlomieju could you review this PR? I want to implement assertSnapshot after this PR merged. Thanks! 🙂

bartlomieju

LGTM

Add name on testContext

2045266

hyp3rflow requested review from bartlomieju and kitsonk as code owners March 17, 2022 04:25

hyp3rflow mentioned this pull request Mar 17, 2022

snapshot testing #3635

Closed

bartlomieju requested a review from dsherret March 17, 2022 12:53

dsherret approved these changes Mar 17, 2022

View reviewed changes

cli/dts/lib.deno.ns.d.ts Show resolved Hide resolved

hyp3rflow changed the title ~~feat(runtime/testing): Expose test name on TestContext~~ feat(runtime/testing): Expose test name, Add update flag for snapshot testing Mar 17, 2022

hyp3rflow requested a review from dsherret March 17, 2022 15:45

bartlomieju requested changes Mar 17, 2022

View reviewed changes

cli/flags.rs Outdated Show resolved Hide resolved

hyp3rflow mentioned this pull request Mar 17, 2022

feat(testing): Implement "assertSnapshot" denoland/std#2039

Merged

dsherret reviewed Mar 17, 2022

View reviewed changes

cli/dts/lib.deno.ns.d.ts Show resolved Hide resolved

hyp3rflow added 5 commits March 18, 2022 01:16

fix: Fix for substep to get its full identifier

5f0684d

feat(testing): Add origin and use update flag

d53f267

fix: Fix typo of getFullName

4071f36

chore: Add jsdoc for TestContext name

5b6eb6b

Implement Deno.test.teardown

3f7d86c

hyp3rflow force-pushed the feat/test-context-name branch from 3df94ba to 3f7d86c Compare March 17, 2022 16:21

hyp3rflow requested a review from bartlomieju March 17, 2022 16:40

fix update option and not to provide fullname, add parent context

7e057cd

formatting

09dc034

hyp3rflow added 3 commits March 18, 2022 10:40

Remove update mode in runtime/testing

3d243b9

fix jsdoc for name

e59a330

Move jsdoc for teardown method

cf83e4e

change not to introduce unnecessary teardown API

2b81511

dsherret approved these changes Mar 23, 2022

View reviewed changes

hyp3rflow changed the title ~~feat(runtime/testing): Expose test name, test origin, Add test teardown API~~ feat(runtime/testing): Expose test name, test origin Mar 23, 2022

hyp3rflow changed the title ~~feat(runtime/testing): Expose test name, test origin~~ feat(runtime/testing): Expose test name, test origin, parentContext Mar 23, 2022

dsherret reviewed Mar 23, 2022

View reviewed changes

hyp3rflow requested a review from dsherret March 23, 2022 14:53

hyp3rflow commented Mar 23, 2022

View reviewed changes

runtime/js/40_testing.js Outdated Show resolved Hide resolved

dsherret requested changes Mar 23, 2022

View reviewed changes

runtime/js/40_testing.js Outdated Show resolved Hide resolved

cli/dts/lib.deno.ns.d.ts Show resolved Hide resolved

hyp3rflow requested a review from dsherret March 23, 2022 15:45

hyp3rflow and others added 3 commits March 24, 2022 01:22

fix to use existing context object instead of creating new one

222fc0e

Merge branch 'main' into feat/test-context-name

0b35c20

reset CI

99b7d8c

hyp3rflow added 2 commits March 29, 2022 05:42

Rename parentContext to parent

95892ba

Add test for textContext apis

59c97f8

format testing_test

07c8e65

dsherret approved these changes Mar 29, 2022

View reviewed changes

bartlomieju approved these changes Mar 31, 2022

View reviewed changes

Merge branch 'main' into feat/test-context-name

ba1c18f

bartlomieju changed the title ~~feat(runtime/testing): Expose test name, test origin, parentContext~~ feat(test): Add "name", "origin" and "parent" to "Deno.TestContext" Apr 6, 2022

bartlomieju merged commit 0df1854 into denoland:main Apr 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(test): Add "name", "origin" and "parent" to "Deno.TestContext" #14007

feat(test): Add "name", "origin" and "parent" to "Deno.TestContext" #14007

hyp3rflow commented Mar 17, 2022 •

edited

Loading

CLAassistant commented Mar 17, 2022 •

edited

Loading

dsherret left a comment •

edited

Loading

hyp3rflow commented Mar 17, 2022

dsherret left a comment

hyp3rflow commented Mar 17, 2022 •

edited

Loading

kitsonk commented Mar 17, 2022 •

edited

Loading

bartlomieju commented Mar 17, 2022

kitsonk commented Mar 17, 2022

hyp3rflow commented Mar 18, 2022 •

edited

Loading

hyp3rflow commented Mar 19, 2022 •

edited

Loading

bcheidemann commented Mar 19, 2022

hyp3rflow commented Mar 19, 2022

hyp3rflow commented Mar 22, 2022 •

edited

Loading

bartlomieju commented Mar 22, 2022

dsherret left a comment •

edited

Loading

dsherret left a comment •

edited

Loading

hyp3rflow commented Mar 23, 2022

dsherret left a comment

hyp3rflow commented Mar 25, 2022

dsherret commented Mar 28, 2022

hyp3rflow commented Mar 28, 2022

dsherret left a comment

dsherret Mar 29, 2022

hyp3rflow commented Mar 31, 2022

bartlomieju left a comment

feat(test): Add "name", "origin" and "parent" to "Deno.TestContext" #14007

feat(test): Add "name", "origin" and "parent" to "Deno.TestContext" #14007

Conversation

hyp3rflow commented Mar 17, 2022 • edited Loading

CLAassistant commented Mar 17, 2022 • edited Loading

dsherret left a comment • edited Loading

Choose a reason for hiding this comment

hyp3rflow commented Mar 17, 2022

dsherret left a comment

Choose a reason for hiding this comment

hyp3rflow commented Mar 17, 2022 • edited Loading

kitsonk commented Mar 17, 2022 • edited Loading

bartlomieju commented Mar 17, 2022

kitsonk commented Mar 17, 2022

hyp3rflow commented Mar 18, 2022 • edited Loading

hyp3rflow commented Mar 19, 2022 • edited Loading

bcheidemann commented Mar 19, 2022

hyp3rflow commented Mar 19, 2022

hyp3rflow commented Mar 22, 2022 • edited Loading

bartlomieju commented Mar 22, 2022

dsherret left a comment • edited Loading

Choose a reason for hiding this comment

dsherret left a comment • edited Loading

Choose a reason for hiding this comment

hyp3rflow commented Mar 23, 2022

dsherret left a comment

Choose a reason for hiding this comment

hyp3rflow commented Mar 25, 2022

dsherret commented Mar 28, 2022

hyp3rflow commented Mar 28, 2022

dsherret left a comment

Choose a reason for hiding this comment

dsherret Mar 29, 2022

Choose a reason for hiding this comment

hyp3rflow commented Mar 31, 2022

bartlomieju left a comment

Choose a reason for hiding this comment

hyp3rflow commented Mar 17, 2022 •

edited

Loading

CLAassistant commented Mar 17, 2022 •

edited

Loading

dsherret left a comment •

edited

Loading

hyp3rflow commented Mar 17, 2022 •

edited

Loading

kitsonk commented Mar 17, 2022 •

edited

Loading

hyp3rflow commented Mar 18, 2022 •

edited

Loading

hyp3rflow commented Mar 19, 2022 •

edited

Loading

hyp3rflow commented Mar 22, 2022 •

edited

Loading

dsherret left a comment •

edited

Loading

dsherret left a comment •

edited

Loading