Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 19 commits into from
Apr 6, 2022
Merged

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
2045266
Add name on testContext
hyp3rflow Mar 17, 2022
5f0684d
fix: Fix for substep to get its full identifier
hyp3rflow Mar 17, 2022
d53f267
feat(testing): Add origin and use update flag
hyp3rflow Mar 17, 2022
4071f36
fix: Fix typo of getFullName
hyp3rflow Mar 17, 2022
5b6eb6b
chore: Add jsdoc for TestContext name
hyp3rflow Mar 17, 2022
3f7d86c
Implement `Deno.test.teardown`
hyp3rflow Mar 17, 2022
7e057cd
fix update option and not to provide fullname, add parent context
hyp3rflow Mar 17, 2022
09dc034
formatting
hyp3rflow Mar 18, 2022
3d243b9
Remove update mode in runtime/testing
hyp3rflow Mar 18, 2022
e59a330
fix jsdoc for name
hyp3rflow Mar 18, 2022
cf83e4e
Move jsdoc for teardown method
hyp3rflow Mar 18, 2022
2b81511
change not to introduce unnecessary teardown API
hyp3rflow Mar 23, 2022
222fc0e
fix to use existing context object instead of creating new one
hyp3rflow Mar 23, 2022
0b35c20
Merge branch 'main' into feat/test-context-name
bartlomieju Mar 24, 2022
99b7d8c
reset CI
bartlomieju Mar 24, 2022
95892ba
Rename parentContext to parent
hyp3rflow Mar 28, 2022
59c97f8
Add test for textContext apis
hyp3rflow Mar 28, 2022
07c8e65
format testing_test
hyp3rflow Mar 29, 2022
ba1c18f
Merge branch 'main' into feat/test-context-name
bartlomieju Apr 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions cli/dts/lib.deno.ns.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,19 @@ declare namespace Deno {
}

export interface TestContext {
/**
* The current test name.
*/
name: string;
dsherret marked this conversation as resolved.
Show resolved Hide resolved
dsherret marked this conversation as resolved.
Show resolved Hide resolved
/**
* File Uri of the current test code.
*/
origin: string;
dsherret marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* Parent test context.
*/
parent?: TestContext;

/** Run a sub step of the parent test or step. Returns a promise
* that resolves to a boolean signifying if the step completed successfully.
* The returned promise never rejects unless the arguments are invalid.
Expand All @@ -270,6 +283,9 @@ declare namespace Deno {

export interface TestStepDefinition {
fn: (t: TestContext) => void | Promise<void>;
/**
* The current test name.
*/
name: string;
ignore?: boolean;
/** Check that the number of async completed ops after the test step is the same
Expand All @@ -287,6 +303,9 @@ declare namespace Deno {

export interface TestDefinition {
fn: (t: TestContext) => void | Promise<void>;
/**
* The current test name.
*/
name: string;
ignore?: boolean;
/** If at least one test has `only` set to true, only run tests that have
Expand Down Expand Up @@ -467,6 +486,13 @@ declare namespace Deno {
*/
export function exit(code?: number): never;

namespace test {
/**
* After the test of the module, registered function will be called.
*/
export function teardown(fn: () => void | Promise<void>): void;
}

export const env: {
/** Retrieve the value of an environment variable. Returns `undefined` if that
* key doesn't exist.
Expand Down
24 changes: 23 additions & 1 deletion runtime/js/40_testing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -985,6 +985,8 @@
reportTestResult(description, result, elapsed);
}

if (teardownFn) await teardownFn();

globalThis.console = originalConsole;
}

Expand Down Expand Up @@ -1046,7 +1048,7 @@
* }} TestStepDefinition
*
* @typedef {{
* name: string;
* name: string,
* parent: TestStep | undefined,
* rootTestDescription: { origin: string; name: string };
* sanitizeOps: boolean,
Expand Down Expand Up @@ -1250,6 +1252,20 @@
function createTestContext(parentStep) {
return {
[SymbolToStringTag]: "TestContext",
/**
* The current test name.
*/
name: parentStep.name,
/**
* Parent test context.
*/
parent: parentStep.parent
? createTestContext(parentStep.parent)
dsherret marked this conversation as resolved.
Show resolved Hide resolved
: undefined,
hyp3rflow marked this conversation as resolved.
Show resolved Hide resolved
/**
* File Uri of the test code.
*/
origin: parentStep.rootTestDescription.origin,
/**
* @param nameOrTestDefinition {string | TestStepDefinition}
* @param fn {(t: TestContext) => void | Promise<void>}
Expand Down Expand Up @@ -1408,6 +1424,12 @@
return value == null ? defaultValue : value;
}

let teardownFn;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to consider that test.teardown might be called multiple times? This could lead to some very unexpected and hard to debug behaviour with the current implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to consider that case. Is there any good idea for handling that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about this? push teardownFn and call sequentially after the test.

const teardownFns = [];
function teardown(fn) {
	if(!teardownFns.includes(fn)) teardownFns.push(fn);
}

// after the test
for await (const fn of teardownFns) {
	await fn();
}

but I'm not sure this is good about the order of calls.
To handle the order problem, we can take another approach.

let teardownFn = () => {};
function teardown(fn) {
	teardownFn = fn(teardownFn);
}

// example of fn
function exampleFn(alreadyRegisteredFn) {
	return async function () {
		// await alreadyRegisteredFn();
		doAwesomeStuff();
		// await alreadyRegisteredFn(); <- we can choose the call order of `teardownFns`.
	}
}

Copy link
Contributor

@bcheidemann bcheidemann Mar 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good point!

I think the second example you gave could be improved in the following ways:

  1. It is hard to understand the order of calls because developers must check each teardown function to know what the order is.
  2. It is prone to errors e.g. a developer forgets to call the previous teardown function or calls it twice

The issue is that each test has a degree of control over the order of teardown calls, essentially democratising the order. This approach probably makes problem #1 unsolvable.

How important is it that developers have control over the order of calls? We can still go with your first suggestion and if fine grained control over the order is important, then it will be possible to implement a custom solution:

function doTeardownOperationsInSomeOrder() {
  // --snip--
}

Deno.test("name", (test) => {
  test.teardown(doTeardownOperationsInSomeOrder);

  test.step("stepName", (test) => {
    // Custom solution to register teardown operations to be called by `doTeardownOperationsInSomeOrder`
  }

  // --snip--
});

However, I believe that in 99% of cases this problem would be solved by Deno exposing the following functions in the test context:

Deno.test("test", (test) => {
  test.beforeAll(() => { /* --snip-- */ });
  test.beforeEach(() => { /* --snip-- */ });
  test.afterAll(() => { /* --snip-- */ });
  test.afterEach(() => { /* --snip-- */ });
});

These would work similarly to the Jest Setup and Teardown functions and allow for a decent level of control over the order while still being easy to understand.

To summarise, I propose the following:

test.beforeAll - [NOT IN THIS PR] execute a function before any child steps are executed
test.afterAll - [NOT IN THIS PR] execute a function after any child steps are executed
test.beforeEach - [NOT IN THIS PR] execute a function before each child steps is executed
test.afterEach - [NOT IN THIS PR] execute a function after each child steps is executed
test.teardown - [THIS PR] execute a function after the top level parent test has finished executing and all descendant test steps have finished executing

What do you think @hyp3rflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the kind review!
Yeah, I think it will be okay with my first suggestion in most cases. I was just worried about if there are many third-party libraries using teardown in an order-sensitive way.
I also agree that it will be awesome with APIs like jest setup-teardown functions. but I think we need to hear the maintainers' thoughts about this kind of API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I think we need to hear the maintainers' thoughts about this kind of API.

Definitely agree! And this should not be part of this PR either way. I just mentioned it here as it would address the ordering issue you flagged.

I was just worried about if there are many third-party libraries using teardown in an order-sensitive way.

I am not aware of any but it's a valid point.

Yeah, I think it will be okay with my first suggestion in most cases.

I agree! Better to keep it simple :D

Copy link
Contributor Author

@hyp3rflow hyp3rflow Mar 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcheidemann I have a question.

test.teardown - [THIS PR] execute a function after the top level parent test has finished executing and all descendant test steps have finished executing.

What does top level parent test means? A single deno test (Deno.test()) or single test module (a.test.ts)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcheidemann I have a question.

test.teardown - [THIS PR] execute a function after the top level parent test has finished executing and all descendant test steps have finished executing.

What does top level parent test means? A single deno test (Deno.test()) or single test module (a.test.ts)?

I was thinking a single Deno.test() but what do you think is better?

For our case (assertSnapshot) it probably makes more sense for it to be a single test module (a.test.ts) but I think generally it makes more sense for this to relate to a single Deno.test() instance.

function teardown(fn) {
teardownFn = fn;
}
test.teardown = teardown;

window.__bootstrap.internals = {
...window.__bootstrap.internals ?? {},
runTests,
Expand Down