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(jupyter): support Deno.test() #20778

Merged
merged 19 commits into from
Oct 5, 2023
Merged

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented Oct 4, 2023

Blocked by #20783.

image

image

@nayeemrmn nayeemrmn changed the title feat(repl): support Deno.test() feat(jupyter): support Deno.test() Oct 4, 2023
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.

There's quite a few moving parts here - would you be able to split this PR into two separate PRs so that if something goes wrong we can revert them in bite-sized chunks? I believe the refactor or tools/test to accept writer argument could be a standalone PR which would be a nice cleanup anyway. Would you be able to update cli/tests/testdata/jupyter/integration_tests.ipynb to include a cell that runs a test?

cli/tools/repl/session.rs Outdated Show resolved Hide resolved
@rgbkrk
Copy link
Contributor

rgbkrk commented Oct 4, 2023

I am so excited about this because now we can give tutorials on writing tests within a notebook. 👨🏻‍🍳💋

@nayeemrmn nayeemrmn requested a review from bartlomieju October 5, 2023 19:06
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.

Overall looks good, nearly there. I have one question regarding output capture

cli/tools/jupyter/mod.rs Outdated Show resolved Hide resolved
ops::jupyter::deno_jupyter::init_ops(stdio_tx.clone()),
crate::ops::testing::deno_test::init_ops(test_event_sender.clone()),
],
// FIXME(nayeemrmn): Test output capturing currently doesn't work.
Copy link
Member

Choose a reason for hiding this comment

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

Hm, shouldn't we be able to capture the test output the same way deno test does it? Isn't it a matter of forwarding it to a proper channel?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It causes all logged output in the repl to be buffered and then flushed to stdout at once when a test event is processed.

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, let's try to address the capturing soon

@nayeemrmn nayeemrmn merged commit ac464ea into denoland:main Oct 5, 2023
@nayeemrmn nayeemrmn deleted the repl-unit-tests branch October 5, 2023 23:03
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.

3 participants