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): explicit resource management for spy() and stub() #4306

Merged
merged 5 commits into from Feb 16, 2024

Conversation

KyleJune
Copy link
Contributor

Closes #3966

This is a work in progress. It won't work until swc is upgraded to v1.4.0 because earlier versions do not support the using keyword on functions. Until that issue is resolved, the new test cases I added fail with the following errors.

image

Once that issue is resolved, I believe the new test cases in this PR will pass and we could go forward with shipping it.

Regarding documentation, I added a description to both the spy and stub sections to explain that they are disposable. I noticed that FakeTime was already made disposible and it's example uses the using keyword. I thought it would be better to explicitly explain that since using is a new feature to TypeScript. I think if all the examples were updated to just use the using keyword to automate cleanup, people could easily miss the importance of that keyword in ensuring that the tests are cleaned up properly. Feel free to change my descriptions though.

testing/time.ts Outdated Show resolved Hide resolved
@iuioiua
Copy link
Collaborator

iuioiua commented Feb 11, 2024

Note to self: run CI when the required functionality is available in canary. Tests should pass in canary but fail in stable.

@iuioiua
Copy link
Collaborator

iuioiua commented Feb 12, 2024

Note to self: run CI when the required functionality is available in canary. Tests should pass in canary but fail in stable.

I didn't notice, but this is already the case! 😆 We've already upgraded to swc@1.4.0. I'll review this in time for the next release of deno_std.

@iuioiua
Copy link
Collaborator

iuioiua commented Feb 12, 2024

@KyleJune, would you also like to update any applicable spies and stubs across the codebase to use the using keyword, too?

@babiabeo
Copy link
Contributor

I think we should also add a note about the minimum Deno version required for it to work. 👍

@github-actions github-actions bot added the http label Feb 13, 2024
@KyleJune
Copy link
Contributor Author

@KyleJune, would you also like to update any applicable spies and stubs across the codebase to use the using keyword, too?

Done, the only ones outside of the context of testing testing/mock_test.ts and testing/time_test.ts were testing/bdd_test.ts and http/file_server_test.ts. I didn't update the mock and time tests since the restore abilities are part of what is being tested.

I think we should also add a note about the minimum Deno version required for it to work. 👍

I don't think that is necessary and that wasn't done when FakeTime was updated to have explicit resource management. I don't know if the next version will be v1.40.5 or v1.41.0 so I'm not going to update it to say which version is required. If the core members want to update it to specify the minimum version required for it, they can make the change.

@babiabeo
Copy link
Contributor

babiabeo commented Feb 15, 2024

I think this PR is now ready to be reviewed as Deno v1.40.5 supports using with function.

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. Looks a nice feature!

Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

This is awesome. Thank you! LGTM.

@iuioiua iuioiua changed the title feat(testing): Explicit resource management for spy() and stub() feat(testing): explicit resource management for spy() and stub() Feb 16, 2024
@iuioiua iuioiua merged commit c6ed026 into denoland:main Feb 16, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

investigate(testing): explicit resource management for spy() and stub()
4 participants