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

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

Closed
bartlomieju opened this issue Mar 3, 2022 · 16 comments · Fixed by #2067

Comments

@bartlomieju
Copy link
Member

Discussed in https://github.com/denoland/deno_std/discussions/1942

Originally posted by KyleJune February 20, 2022
In the discord, @bartlomieju suggested opening an issue here about possibly adding my third party module to deno_std to help bridge a gap in testing experience between Deno and Node.

The test_suite module is not a direct port of Jasmine, Jest, or Mocha but has similar functionality. The describe and it functions take a test definition that is an extension of Deno.TestDefinition. For the describe and it functions, I copied all the call signatures Deno has for Deno.test and t.step.

I implemented the only option for nested tests which you currently cannot do with the t.step.

It depends on 1 third party module I wrote called collections. It just uses the Vector class from there instead of Array to make it easier to remove steps when a new test step is focused. It wouldn't be difficult to re-write the parts that use Vector to not use it.

The test coverage depends on another third party module I wrote called mock. It is not a direct port of sinon.js but is similar. I wrote it from scratch in TypeScript for Deno because when I originally started using Deno, I wasn't able to get sinon.js to work.

I have 92.90% test coverage currently. For getting rid of the third party module dependency, they would need to be re-written. Currently the testing works by mocking Deno.test and creating a fake TestContext. @bartlomieju said he thinks this could be verified by integration tests - spawning a subprocesses and checking if output matches instead of using mocking.

More information and examples can be found in the readme here.

The interface is documented using Deno Doc.

@kt3k
Copy link
Member

kt3k commented Mar 4, 2022

I'm in favor of this proposal. I recently ported many npm modules from crypto-browserify to std, and converting all describe/it to Deno.test/await t.step was very painful work. If this module is available in std, I think it should promote the migration of node.js code to deno-first project.

(links to past similar discussions: #1389, #931, #1831)

Regarding the past discussions/objections, maybe let's not try to make it compatible to mocha/jasmine/jest, but just make it 'enough similar' to them. How about include this as std/testing/bdd.ts?

@KyleJune
Copy link
Contributor

KyleJune commented Mar 4, 2022

I'm in favor of this proposal. I recently ported many npm modules from crypto-browserify to std, and converting all describe/it to Deno.test/await t.step was very painful work. If this module is available in std, I think it should promote the migration of node.js code to deno-first project.

(links to past similar discussions: #1389, #931, #1831)

Regarding the past discussions/objections, maybe let's not try to make it compatible to mocha/jasmine/jest, but just make it 'enough similar' to them. How about include this as std/testing/bdd.ts?

It's close enough that it should be easy to adapt existing mocha, jest, and jasmine tests to using describe and it from these functions. I believe in most cases it would be as simple as adding the import at the top. For hooks you may need to rename before to beforeAll and same for after, depending on which test runner you were using. I went with including all in that hook name to make it more clear that it's not beforeEach.

I'm not sure about the other test libraries but currently mine has a limitation of calling the before and after hooks once per describe and at the top level. It also requires them to be called at the top of the describe block so that it is more clear the hooks apply to all subtests. It would be easy to adapt to allow hooks to be registered after tests are in a describe block. If would also be easy to change it to allow you to call the hook functions multiple times per describe block.

One issue I currently have is with nested only flag, I can't modify the top level test because it registers immediately, currently if you add the only flag to a nested test, it will omit all nested test cases within the top level test that do not have to the only flag. I couldn't retroactively add only to top level tests that had already been registered with Deno.test. If there was a way to trigger registering at the end right before tests start running, I could resolve the issue.

@KyleJune
Copy link
Contributor

KyleJune commented Mar 4, 2022

The fact that the describe and it definitions extend Deno.TestDefinition and has all the same call signatures minus the fn being different makes it so that it would be easy to switch from using Deno.test and t.step to using describe and it.

@KyleJune
Copy link
Contributor

KyleJune commented Mar 4, 2022

One difference between this module and the node test frameworks is that this one also supports a flat style, an example of that can be found in the readme. What it means is that describe does not have to take a fn callback and will return an object that can be used to add tests to the group. The it and describe functions optionally take that suite object as a first argument or it can be included in it's definition to signal it belongs to the describe test block.

The motivation for having the flat style is to make it easy to have deeper subgroups without the lines getting to long. With deno fmt line limit, it can make the dealer tests more difficult to read due to them having to be broken apart across more lines due to the amount of indentation they have.

@KyleJune
Copy link
Contributor

KyleJune commented Mar 6, 2022

If I could get an example of a test case that would be acceptable without using mocking, I could get started on re-writing the tests so that it will be easier to add this module to deno_std later on. I'll wait to do the actual work until it's agreed on that it should be added or in what way it should be added.

I've created a few issues in the test_suite module's github repository regarding improving the module and preparing it for being able to be added to deno_std.

https://github.com/udibo/test_suite/issues

@KyleJune
Copy link
Contributor

KyleJune commented Mar 7, 2022

I've cleaned up the TestSuite interface, hiding all the functions and properties that are used internally only. The TestSuite object now just has a symbol on it for looking up the internal representation for a group of tests.

I've removed the third party dependency on Vector that was mentioned in my original post.

@kt3k
Copy link
Member

kt3k commented Mar 7, 2022

I've cleaned up the TestSuite interface, hiding all the functions and properties that are used internally only.

Nice!

I've removed the third party dependency on Vector that was mentioned in my original post.

👍

I could get started on re-writing the tests so that it will be easier to add this module to deno_std later on.

How much do you expect that re-write would take amount of time? If it takes a lot of time, then I guess including the entire mock module as test utility in std would be a possible option.

@KyleJune
Copy link
Contributor

KyleJune commented Mar 7, 2022

I could get started on re-writing the tests so that it will be easier to add this module to deno_std later on.

How much do you expect that re-write would take amount of time? If it takes a lot of time, then I guess including the entire mock module as test utility in std would be a possible option.

It depends on what the new tests would need to look like, I'd need an example to estimate re-wrote time. Also I mostly just do work on side projects like this on weekends because I work as a software engineer full time during the week.

The mock module is similar to Sinon.js but I wrote it from scratch in TypeScript. I'm not sure how much work it would take to get it into a state where it would be accepted for being added to deno_std.

It also currently depends on my third party collections module like test_suite previously did. Changing it from using Vector to Array wouldn't be hard. But it also uses a Red-Black tree I wrote for accurately copying the behavior of Deno's timers in mocks FakeTime. Deno has or had a Red-Black Tree in it's JavaScript code for internal use only at one point.

https://deno.land/x/collections@0.11.2/trees/rb_tree.ts

If Deno had a Red-Black Tree in it's std modules it would make it easier to adapt mock to no longer having third party dependencies.

I think some of the data structures I implemented in my collections module would be good to add to std/collections. JavaScript doesn't have a double ended queue, binary heap, or red black tree. Other languages like Rust and Java have standard double ended queues and binary heaps. I'm not sure if any other languages have standard red black trees though. JavaScript`s Array can serve as a double ended queue because it has shift and unshift functions, but they are not efficient to use because every call triggers reindexing the array. My collections module has 98% test coverage, it's just missing a few functions on the Vector class that I would like it to have but just haven't gotten around to doing due to having other higher priority projects I'd like to work on.

@KyleJune
Copy link
Contributor

KyleJune commented Mar 7, 2022

I feel like re-writing the tests in the way @bartlomieju described would probably make this require much more test coverage code than it currently has because we would probably want to also verify all of the different TestDefinition and TestStepDefinition properties are being covered on describe and it.

The current test coverage just verified that properties like sanitizeOps are being passed through to Deno.test and t.step calls correctly instead of verifying that ops are being sanitized correctly by describe and it, since it can assume that those properties work correctly if passed through correctly.

@bartlomieju
Copy link
Member Author

@KyleJune sorry for late response. Let me try to address as many details as possible.

I could get started on re-writing the tests so that it will be easier to add this module to deno_std later on.

How much do you expect that re-write would take amount of time? If it takes a lot of time, then I guess including the entire mock module as test utility in std would be a possible option.

It depends on what the new tests would need to look like, I'd need an example to estimate re-wrote time. Also I mostly just do work on side projects like this on weekends because I work as a software engineer full time during the week.

The mock module is similar to Sinon.js but I wrote it from scratch in TypeScript. I'm not sure how much work it would take to get it into a state where it would be accepted for being added to deno_std.

I think we could consider adding mock to std/testing too - seems very useful and a lot of folks use such functionality, I think it warrants addition to deno_std.

It also currently depends on my third party collections module like test_suite previously did. Changing it from using Vector to Array wouldn't be hard. But it also uses a Red-Black tree I wrote for accurately copying the behavior of Deno's timers in mocks FakeTime. Deno has or had a Red-Black Tree in it's JavaScript code for internal use only at one point.

https://deno.land/x/collections@0.11.2/trees/rb_tree.ts

If Deno had a Red-Black Tree in it's std modules it would make it easier to adapt mock to no longer having third party dependencies.

I think some of the data structures I implemented in my collections module would be good to add to std/collections. JavaScript doesn't have a double ended queue, binary heap, or red black tree. Other languages like Rust and Java have standard double ended queues and binary heaps. I'm not sure if any other languages have standard red black trees though. JavaScript`s Array can serve as a double ended queue because it has shift and unshift functions, but they are not efficient to use because every call triggers reindexing the array. My collections module has 98% test coverage, it's just missing a few functions on the Vector class that I would like it to have but just haven't gotten around to doing due to having other higher priority projects I'd like to work on.

Correct, our implementation of RB tree is internal and can't be exposed. I'm also open to adding this module to std/collections as well as other structures you have there. They seem very handy 👍

I feel like re-writing the tests in the way @bartlomieju described would probably make this require much more test coverage code than it currently has because we would probably want to also verify all of the different TestDefinition and TestStepDefinition properties are being covered on describe and it.

The current test coverage just verified that properties like sanitizeOps are being passed through to Deno.test and t.step calls correctly instead of verifying that ops are being sanitized correctly by describe and it, since it can assume that those properties work correctly if passed through correctly.

We can do this a multi-step process. There were requests to add snapshot testing utility to deno_std and I think we should do that. Such functionality would greatly help with testing your code.

One difference between this module and the node test frameworks is that this one also supports a flat style, an example of that can be found in the readme. What it means is that describe does not have to take a fn callback and will return an object that can be used to add tests to the group. The it and describe functions optionally take that suite object as a first argument or it can be included in it's definition to signal it belongs to the describe test block.

The motivation for having the flat style is to make it easy to have deeper subgroups without the lines getting to long. With deno fmt line limit, it can make the dealer tests more difficult to read due to them having to be broken apart across more lines due to the amount of indentation they have.

Is this something that could be changed to preserve behavior of Node's testing framework with multi-level grouping? The fewer changes users migrating would have to apply to better. There's also a quick and dirty polyfill written by @lucacasonato some time ago (https://gist.github.com/lucacasonato/54c03bb267074aaa9b32415dbfb25522).

@bartlomieju
Copy link
Member Author

Also ref #1389

@KyleJune
Copy link
Contributor

KyleJune commented Mar 8, 2022

The mock module is similar to Sinon.js but I wrote it from scratch in TypeScript. I'm not sure how much work it would take to get it into a state where it would be accepted for being added to deno_std.

I think we could consider adding mock to std/testing too - seems very useful and a lot of folks use such functionality, I think it warrants addition to deno_std.

I could work on adding that after adding the data structures it uses to std/collections.

I think some of the data structures I implemented in my collections module would be good to add to std/collections. JavaScript doesn't have a double ended queue, binary heap, or red black tree. Other languages like Rust and Java have standard double ended queues and binary heaps. I'm not sure if any other languages have standard red black trees though. JavaScript`s Array can serve as a double ended queue because it has shift and unshift functions, but they are not efficient to use because every call triggers reindexing the array. My collections module has 98% test coverage, it's just missing a few functions on the Vector class that I would like it to have but just haven't gotten around to doing due to having other higher priority projects I'd like to work on.

Correct, our implementation of RB tree is internal and can't be exposed. I'm also open to adding this module to std/collections as well as other structures you have there. They seem very handy +1

I'll try getting some PRs up this weekend for that if I have time to. Then after getting it and mock added, do the work to add test_suite.

I feel like re-writing the tests in the way @bartlomieju described would probably make this require much more test coverage code than it currently has because we would probably want to also verify all of the different TestDefinition and TestStepDefinition properties are being covered on describe and it.

The current test coverage just verified that properties like sanitizeOps are being passed through to Deno.test and t.step calls correctly instead of verifying that ops are being sanitized correctly by describe and it, since it can assume that those properties work correctly if passed through correctly.

We can do this a multi-step process. There were requests to add snapshot testing utility to deno_std and I think we should do that. Such functionality would greatly help with testing your code.

I'm not too familiar with snapshot testing. If that capability is added and I'm given an example of how to use it, I could re-write the tests to use it for verifying the tests get registered correctly instead of using mocking.

One difference between this module and the node test frameworks is that this one also supports a flat style, an example of that can be found in the readme. What it means is that describe does not have to take a fn callback and will return an object that can be used to add tests to the group. The it and describe functions optionally take that suite object as a first argument or it can be included in it's definition to signal it belongs to the describe test block.

The motivation for having the flat style is to make it easy to have deeper subgroups without the lines getting to long. With deno fmt line limit, it can make the dealer tests more difficult to read due to them having to be broken apart across more lines due to the amount of indentation they have.

Is this something that could be changed to preserve behavior of Node's testing framework with multi-level grouping? The fewer changes users migrating would have to apply to better. There's also a quick and dirty polyfill written by @lucacasonato some time ago (https://gist.github.com/lucacasonato/54c03bb267074aaa9b32415dbfb25522).

It's compatible with the node testing framework behavior for multi-level grouping. The difference is basically just that describe will return a reference to the test group that can be passed into other other describe/it function calls to make them children of it. The flat and non flat style of multi-level grouping can be mix and matched. Both styles work and examples of each can be found in the readme.

I believe with the flat style being available, it's easier to start organizing tests by just creating a group with describe then passing it to your existing it calls that you want to belong to the describe test grouping.

The flat style is basically an extra feature that I could strip out if needed to be able to get it into deno_std. I personally prefer using the flat style for my own tests.

@bartlomieju
Copy link
Member Author

I could work on adding that after adding the data structures it uses to std/collections.

I'll try getting some PRs up this weekend for that if I have time to. Then after getting it and mock added, do the work to add test_suite.

Sounds good! Let me know if you need any help.

I'm not too familiar with snapshot testing. If that capability is added and I'm given an example of how to use it, I could re-write the tests to use it for verifying the tests get registered correctly instead of using mocking.

It's essentially a way of asserting that output matches what is expected with some wildcards. Examples:
https://jestjs.io/docs/snapshot-testing

We use this technique heavily in deno itself, eg. https://github.com/denoland/deno/blob/main/cli/tests/testdata/test/overloads.ts and https://github.com/denoland/deno/blob/main/cli/tests/testdata/test/overloads.out

It's compatible with the node testing framework behavior for multi-level grouping. The difference is basically just that describe will return a reference to the test group that can be passed into other other describe/it function calls to make them children of it. The flat and non flat style of multi-level grouping can be mix and matched. Both styles work and examples of each can be found in the readme.

I believe with the flat style being available, it's easier to start organizing tests by just creating a group with describe then passing it to your existing it calls that you want to belong to the describe test grouping.

The flat style is basically an extra feature that I could strip out if needed to be able to get it into deno_std. I personally prefer using the flat style for my own tests.

Perfect, I guess I misunderstood.

@KyleJune
Copy link
Contributor

KyleJune commented Mar 8, 2022

I'm not too familiar with snapshot testing. If that capability is added and I'm given an example of how to use it, I could re-write the tests to use it for verifying the tests get registered correctly instead of using mocking.

It's essentially a way of asserting that output matches what is expected with some wildcards. Examples: https://jestjs.io/docs/snapshot-testing

We use this technique heavily in deno itself, eg. https://github.com/denoland/deno/blob/main/cli/tests/testdata/test/overloads.ts and https://github.com/denoland/deno/blob/main/cli/tests/testdata/test/overloads.out

Oh nice, I get what you mean now. Still with switching to that style, we would probably want to add test coverage that describe and it would pass if not leaking async ops and fail if leaking async ops when sanitizeOps is true and pass when sanitizeOps is false. With mocking we can just verify we pass the sanitizeOps argument through correctly to Deno.test and t.step and assume it works correctly since that code already has good test coverage for all the test options available in the TestDefinition.

@KyleJune
Copy link
Contributor

I've removed the test dependency on the mock module's FakeTime class. That was the one that used the collection module's Red-Black Tree. I'm working on a refactor of the mock module then will make a PR for adding spy and stub to std/testing. Those are all that are needed for the test_suite module's test coverage.

I think FakeTime would be good to add to std/testing too but that could wait until after getting the basic spy/stub functions and describe/it functions into std/testing first.

@KyleJune
Copy link
Contributor

I resolved the issue of not being able to have multiple of the same type of a test hook globally or with a describe block. I'll try to create a PR to add test_suite to std next weekend. It will branch off of my branch for adding mocking utilities since the test coverage uses them.

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 a pull request may close this issue.

3 participants