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.

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(testing): Add behavior-driven development #2067

Merged
merged 20 commits into from
Apr 8, 2022
Merged

Conversation

KyleJune
Copy link
Contributor

@KyleJune KyleJune commented Mar 27, 2022

Closes #1991

Depends on: #2048

All of the commits before feat(testing): Add behavior-driven development belong to the feat(testing): Add mocking utilities PR this one depends on. If I make any more changes to mock, they will be in that PR and I'll re-merge that branch into this one. The reason there are so many call signatures is that I wanted it to be able to match up with all the Deno.test call signatures plus being able to add the suite argument in front of them if you are using the flat grouping style which allows you to add grouping to an existing file with minimal amount of indentation change.

The README.md explains how to use the new bdd.ts module.

Here are the deno docs for bdd.ts. Currently for describe and it functions, I have separate types defining all of the different argument combinations. I wrote it like that to avoid repeating them many times. The deno docs are not expanding the arguments like VS Code does, I created an issue for that. I could update the interfaces and functions to have all the different overloads if that is preferable.

There are a few limitations that I've documented in the README.md file. I don't think any of these limitations are worth holding back this addition but it's worth bringing up in case anyone else has an idea for how to address them that I haven't thought of.

  1. The test step API doesn't support the only option but bdd.ts does support it for grouped tests. However, when they are focused, they will be the only ones to run within the top level test suite. That's because of how the tests are registered and being unable to update previously tests that were registered with Deno.test. I have one idea for how to resolve that issue, if I could hook into when the test runner is about to run the tests, I could delay registering them until then and set the only flag on the Deno.TestDefinition for the top level test that has a sub test with the only flag.
  2. To add hooks at the top level, I have it register a single test case and have all other test cases and test suites be nested within it. That way if there is an issue like leaking ops in the top level hooks, it will be able to catch them like it would in any other test suite with hooks. To be able to add top level hooks, at least one of them have to be registered before any tests or test suites are, since they register top level tests immediately with Deno.test and I can't change how they are registered after they are already registered. The idea I had for fixing the first issue I mentioned would also work for fixing this issue.
  3. The test step API doesn't support the permissions option. The describe and it will register tests using Deno.test if they are at the top level, so that permission option is available for both of those functions. However, when they belong to a test suite, the permissions option will not work because test's belonging to a test suite are registered with the test step API. Because of that, I have it throw an error when you try to use the permissions option in that case. I don't think we need to fix this issue or if we do the fix should be made in the test step API.

@KyleJune
Copy link
Contributor Author

The changes I just merged are from my mock PR. I just reorganized the files for it.

@KyleJune
Copy link
Contributor Author

I updated my mock PR to add a section about it to the README.md file. When merging, I put the new mock section after the section about behavior-driven development that this PR adds.

@KyleJune
Copy link
Contributor Author

I merged main back into this branch. Now this PR only has the changes for adding bdd.ts.

@KyleJune
Copy link
Contributor Author

KyleJune commented Apr 3, 2022

I just fixed a small issue regarding the only option on nested tests. I forgot to make it update parent test suites regarding one of their child test suites having the only option. Because of that, only was only applying to the current suite.

It doesn't fix the issue mentioned in my PR regarding not being able to update the top level test to have the only option if one of its children have an only flag.

@KyleJune
Copy link
Contributor Author

KyleJune commented Apr 3, 2022

I fixed one other issue related to my last change. If multiple tests were focused with only within different child test suites, only the test cases focused in the first test suite would run. I've manually tested this in one of my other projects and only is working correctly now when there are multiple nested tests focused at within different suites.

There are 4 types of hooks available for test suites. A test suite can have
multiples of each type of hook, they will be called in the order that they are
registered. The `afterEach` and `afterAll` hooks will be called whether or not
the test case passes. The all hooks will be called once for the whole group
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should write 'All' hooks instead of all hooks to make it clear what are referenced

Copy link
Member

Choose a reason for hiding this comment

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

*All hooks

There is currently one limitation to this, you cannot use the permissions option
on an individual test case or test suite that belongs to another test suite.

### Migrating and usage
Copy link
Member

Choose a reason for hiding this comment

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

Comparing describe / it APIs with Deno.test is very good, but we generally don't recommend migrating from Deno.test suites to BDD style testing (Deno.test should still be the default way of writing tests and BDD style should be an optional convenient way of writing them if the users prefer) So the section name here sounds concerning to me.

Could you rephrase the section name to something like Comparing to Deno.test?

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'm fine with that change. I just worded it that way as a way to describe how someone could switch to using bdd.ts if they prefer this style over the default style that they may already be using at the time of finding this. I'll try rewriting this in a way that makes it clear this isn't the preferred way of writing tests for Deno and that it's just another way of writing them.

Comment on lines 289 to 290
`it` functions have similar call signatures to `Deno.test`, making it easy to
migrate from using `Deno.test`.
Copy link
Member

Choose a reason for hiding this comment

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

Let's drop the last clause as we generally don't recommend migration from Deno.test to BDD (more descriptions are in the below comment)

Suggested change
`it` functions have similar call signatures to `Deno.test`, making it easy to
migrate from using `Deno.test`.
`it` functions have similar call signatures to `Deno.test`.

});

await t.step("it", async (t) => {
async function assertOptions<T>(
Copy link
Member

Choose a reason for hiding this comment

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

This test utility looks very complex. Please add doc comment which describes the purpose of this and what is checked with this.

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 can do that. These are just used internally. Basically every step in this block of tests have similar assertions regarding Deno.test getting called when you call it, and it verifies Deno.test gets called with the correct options.

Copy link
Member

Choose a reason for hiding this comment

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

These are just used internally.

Yes, but such comments are useful for the maintainers and contributors to better understand the test cases. I'd appreciate if the doc comment is added to these utils, especially with description about what is expected to be passed as cb in them

});

await t.step("describe", async (t) => {
async function assertOptions(
Copy link
Member

Choose a reason for hiding this comment

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

ditto: This test utility looks very complex. Please add doc comment which describes the purpose of this and what is checked with this.

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 can do that. These are just used internally. Basically every step in this block of tests have similar assertions regarding Deno.test and t.step getting called when you call describe and it, and this verifies Deno.test and t.step get called with the correct options.

});

await t.step("with hooks", async (t) => {
async function assertHooks(
Copy link
Member

Choose a reason for hiding this comment

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

ditto: This test utility looks very complex. Please add doc comment which describes the purpose of this and what is checked with this.

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 can do that. These are just used internally. Basically every step in this block of tests have similar assertions regarding Deno.test and t.step getting called when you call describe and it, and this verifies Deno.test and t.step get called with the correct options. It also verified that the hooks were all called in the correct order.

@kt3k
Copy link
Member

kt3k commented Apr 4, 2022

The implementation looks good to me.

multiples of each type of hook, they will be called in the order that they are
registered. The `afterEach` and `afterAll` hooks will be called whether or not
the test case passes. The all hooks will be called once for the whole group
while the each hooks will be called for each individual test case.
Copy link
Member

Choose a reason for hiding this comment

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

*Each hooks

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Initial pass


### Focusing tests

If you would like to only run specific individual test cases, you can do so by
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you would like to only run specific individual test cases, you can do so by
If you would like to run only specific test cases, you can do so by

### Focusing tests

If you would like to only run specific individual test cases, you can do so by
calling `it.only` instead of `it`. If you would like to only run specific test
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
calling `it.only` instead of `it`. If you would like to only run specific test
calling `it.only` instead of `it`. If you would like to run only specific test

Comment on lines 315 to 317
There is one limitation to this when the individual test cases or test suites
belong to another test suite, they will be the only ones to run within the top
level test suite.
Copy link
Member

Choose a reason for hiding this comment

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

Could you provide an example here? It's not obviously clear what this sentence means.

Copy link
Contributor Author

@KyleJune KyleJune Apr 5, 2022

Choose a reason for hiding this comment

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

If you look at one of the user test examples I have, this issue can be reproduced by adding another test case or describe block that doesn't belong to the User describe block. If you add .only to the test for getAge that will be the only test case to run within the top level User describe block. The other test cases that do not belong to the User describe block will also still run. currently the only workaround to ensure the user test step is the only one in the file to run is also adding .only to the User describe block so that when Deno.test is called, it will be called with the only flag.

I'll still add an example to readme for this limitation. Just thought I'd try explaining it more here first.

The reason this problem exists is because Deno.test is called the moment describe is called, and at that time, it doesn't know if any of it's steps have the only flag. If I had a hook into when tests are about to run, I could delay registration until the last moment, allowing me to determine if Deno.test should be called with only flag based on if one of it's child steps has the only flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I thought about this more and think I could resolve limitation for test cases registered within the describe callback. It's just the flat style where it wouldn't be known at time of Deno.test call if a step registered after the describe call has the only flag.

### Ignoring tests

If you would like to not run specific individual test cases, you can do so by
calling `it.ignore` instead of `it`. If you would like to only run specific test
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
calling `it.ignore` instead of `it`. If you would like to only run specific test
calling `it.ignore` instead of `it`. If you would like to not run specific test

Like `Deno.TestDefinition`, the `DescribeDefinition` and `ItDefinition` have
sanitization options. They work in the same way.

- sanitizeExit: Ensure the test case does not prematurely cause the process to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- sanitizeExit: Ensure the test case does not prematurely cause the process to
- `sanitizeExit`: Ensure the test case does not prematurely cause the process to


Like `Deno.TestDefinition`, the `DescribeDefintion` and `ItDefinition` have a
permissions option. They specify the permissions that should be used to run an
individual test case or test suite. Set this to "inherit" to keep the calling
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
individual test case or test suite. Set this to "inherit" to keep the calling
individual test case or test suite. Set this to `"inherit"` to keep the calling

Like `Deno.TestDefinition`, the `DescribeDefintion` and `ItDefinition` have a
permissions option. They specify the permissions that should be used to run an
individual test case or test suite. Set this to "inherit" to keep the calling
thread's permissions. Set this to "none" to revoke all permissions.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
thread's permissions. Set this to "none" to revoke all permissions.
thread's permissions. Set this to `"none"` to revoke all permissions.

individual test case or test suite. Set this to "inherit" to keep the calling
thread's permissions. Set this to "none" to revoke all permissions.

Defaults to "inherit".
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Defaults to "inherit".
This setting defaults to `"inherit"`.

testing/_test_suite.ts Show resolved Hide resolved
testing/bdd.ts Show resolved Hide resolved
@bartlomieju
Copy link
Member

This is amazing work @KyleJune! I'm sure users will find it very useful. Please add copyright header to all files.

@KyleJune
Copy link
Contributor Author

KyleJune commented Apr 5, 2022

Thank you. Glad to hear most feedback is just on documentation and adding comments. I'll make changes based on the feedback either one night this week or this weekend if I end up not having time to during the week.

I'll also include updating the mock PR files to have deno copyright comments at the top of the files. That ended up getting merged without them.

@KyleJune
Copy link
Contributor Author

KyleJune commented Apr 7, 2022

I decided not to commit the examples I wrote highlighting the issues with only when using the flat test style because using only in a test case results in it failing. The issue revolves around not knowing if a child test case is focused at the time it is registered using Deno.test. Below are examples demonstrating the 2 issues along with the current workaround required to get the getAge test case to be one of the only ones that run.


Issue 1: getAge is focused but the Example A and Example B test cases still run. That's because the "User" test was registered with Deno.test without the only option.

import {
  assert,
  assertEquals,
  assertStrictEquals,
  assertThrows,
} from "../asserts.ts";
import { describe, it } from "../bdd.ts";
import { User } from "./user.ts";

it("Example A", () => assert(true));

const userTests = describe("User");

it(userTests, "users initially empty", () => {
  assertEquals(User.users.size, 0);
});

it(userTests, "constructor", () => {
  try {
    const user = new User("Kyle");
    assertEquals(user.name, "Kyle");
    assertStrictEquals(User.users.get("Kyle"), user);
  } finally {
    User.users.clear();
  }
});

const ageTests = describe({
  name: "age",
  suite: userTests,
  beforeEach(this: { user: User }) {
    this.user = new User("Kyle");
  },
  afterEach() {
    User.users.clear();
  },
});

it.only(ageTests, "getAge", function () {
  const { user } = this;
  assertThrows(() => user.getAge(), Error, "Age unknown");
  user.age = 18;
  assertEquals(user.getAge(), 18);
});

it(ageTests, "setAge", function () {
  const { user } = this;
  user.setAge(18);
  assertEquals(user.getAge(), 18);
});

it("Example B", () => assert(true));
kyle@kyle-XPS-15-9560:~/Projects/deno/deno_std$ deno test testing/bdd_examples/user_flat_only_issue_1_test.ts 
running 3 tests from file:///home/kyle/Projects/deno/deno_std/testing/bdd_examples/user_flat_only_issue_1_test.ts
test Example A ... ok (6ms)
test User ...
  test age ...
    test getAge ... ok (5ms)
  ok (9ms)
ok (13ms)
test Example B ... ok (3ms)

test result: ok. 3 passed (2 steps); 0 failed; 0 ignored; 0 measured; 0 filtered out (42ms)

Issue 2: "getAge" and "Example B" are focused but only the "Example B" test cases runs. That's because the "User" test was registered with Deno.test without the only option and the "Example B" test case was registered with Deno.test with the only option. The following example is the same as the one from issue 1 but with the "Example B" test case focused.

import {
  assert,
  assertEquals,
  assertStrictEquals,
  assertThrows,
} from "../asserts.ts";
import { describe, it } from "../bdd.ts";
import { User } from "./user.ts";

it("Example A", () => assert(true));

const userTests = describe("User");

it(userTests, "users initially empty", () => {
  assertEquals(User.users.size, 0);
});

it(userTests, "constructor", () => {
  try {
    const user = new User("Kyle");
    assertEquals(user.name, "Kyle");
    assertStrictEquals(User.users.get("Kyle"), user);
  } finally {
    User.users.clear();
  }
});

const ageTests = describe({
  name: "age",
  suite: userTests,
  beforeEach(this: { user: User }) {
    this.user = new User("Kyle");
  },
  afterEach() {
    User.users.clear();
  },
});

it.only(ageTests, "getAge", function () {
  const { user } = this;
  assertThrows(() => user.getAge(), Error, "Age unknown");
  user.age = 18;
  assertEquals(user.getAge(), 18);
});

it(ageTests, "setAge", function () {
  const { user } = this;
  user.setAge(18);
  assertEquals(user.getAge(), 18);
});

it.only("Example B", () => assert(true));
kyle@kyle-XPS-15-9560:~/Projects/deno/deno_std$ deno test testing/bdd_examples/user_flat_only_issue_2_test.ts 
running 1 test from file:///home/kyle/Projects/deno/deno_std/testing/bdd_examples/user_flat_only_issue_2_test.ts
test Example B ... ok (5ms)

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 2 filtered out (25ms)

error: Test failed because the "only" option was used

Workaround to both of the above issues is to change the describe("User") to describe.only("User") so that it knows to call Deno.test with the only option. The following example is the same as the one from issue 2 but with the "User" test suite focused too.

import {
  assert,
  assertEquals,
  assertStrictEquals,
  assertThrows,
} from "../asserts.ts";
import { describe, it } from "../bdd.ts";
import { User } from "./user.ts";

it("Example A", () => assert(true));

const userTests = describe.only("User");

it(userTests, "users initially empty", () => {
  assertEquals(User.users.size, 0);
});

it(userTests, "constructor", () => {
  try {
    const user = new User("Kyle");
    assertEquals(user.name, "Kyle");
    assertStrictEquals(User.users.get("Kyle"), user);
  } finally {
    User.users.clear();
  }
});

const ageTests = describe({
  name: "age",
  suite: userTests,
  beforeEach(this: { user: User }) {
    this.user = new User("Kyle");
  },
  afterEach() {
    User.users.clear();
  },
});

it.only(ageTests, "getAge", function () {
  const { user } = this;
  assertThrows(() => user.getAge(), Error, "Age unknown");
  user.age = 18;
  assertEquals(user.getAge(), 18);
});

it(ageTests, "setAge", function () {
  const { user } = this;
  user.setAge(18);
  assertEquals(user.getAge(), 18);
});

it.only("Example B", () => assert(true));
kyle@kyle-XPS-15-9560:~/Projects/deno/deno_std$ deno test testing/bdd_examples/user_flat_only_workaround_test.ts 
Check file:///home/kyle/Projects/deno/deno_std/testing/bdd_examples/user_flat_only_workaround_test.ts
running 2 tests from file:///home/kyle/Projects/deno/deno_std/testing/bdd_examples/user_flat_only_workaround_test.ts
test User ...
  test age ...
    test getAge ... ok (4ms)
  ok (7ms)
ok (13ms)
test Example B ... ok (3ms)

test result: ok. 2 passed (2 steps); 0 failed; 0 ignored; 0 measured; 1 filtered out (29ms)

error: Test failed because the "only" option was used

@KyleJune
Copy link
Contributor Author

KyleJune commented Apr 7, 2022

I made the requested documentation changes and added deno copyright to all the bdd and mock files.

I fixed the issue with the only option not working for the nested test grouping style with a small change to _test_suite.ts. I updated test coverage to show that Deno.test is called with the only option set to true when a nested test case or suite is focused. I added test coverage demonstrating it's still an issue when using the flat test grouping style.

@KyleJune
Copy link
Contributor Author

KyleJune commented Apr 7, 2022

Tests were failing because name and origin keys were added to the canary. For my fake TestContext I use in bdd_test.ts, I made the name be the tests name then I wasn't sure what origin is meant to be so I just made it "origin". I don't make any sort of assertions regarding the name or origin key so I don't think it's important for those values to be accurate in the bdd_test.ts file.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, amazing work @KyleJune. I'll wait for @kt3k review before landing.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for working on this! @KyleJune

Let's work on the remaining issues of flat style in follow up PRs

Comment on lines +355 to +359
The default way of writing tests is using `Deno.test` and `t.step`. The
`describe` and `it` functions have similar call signatures to `Deno.test`,
making it easy to switch between the default style and the behavior-driven
development style of writing tests. Internally, `describe` and `it` are
registering tests with `Deno.test` and `t.step`.
Copy link
Member

Choose a reason for hiding this comment

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

Good explanation of our stance towards this! Thanks for updating

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test_suite module to deno_std to help bridge a gap in testing experience between Deno and Node
3 participants