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

refactor: rewrite deno test, add Deno.test() #3865

Merged
merged 27 commits into from
Feb 11, 2020

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Feb 3, 2020

  • rewrite deno test subcommand - no longer requires to download files from internet
  • add cli/testing.ts - this file is a trimmed down copy of std/testing/mod.ts - has only test() and runTests() method
  • Rust test js_unit_test now uses Deno.test() to create all test cases instead of relying on std/testing/mod.ts (this is just some dog-fooding)

deno test does not support globs expansion and exclusion. One needs to pass a full list of files that should be run - ie. you can still call deno test */*_test.ts but it is shell's job to expand the glob.
It's really an MVP to take a step back and look at test module to provide essential things to write test. I expect that third party testing frameworks will arise (or be ported from node). So by no means this PR tries to establish a "batteries-included" testing framework.

cli/js/lib.deno.ns.d.ts Outdated Show resolved Hide resolved
cli/test_runner.rs Outdated Show resolved Hide resolved
@nayeemrmn
Copy link
Collaborator

Once the std test suite is fully converted to use Deno.test() and std/testing/runner.ts is removed, can we make runTests() internal?

@bartlomieju
Copy link
Member Author

Once the std test suite is fully converted to use Deno.test() and std/testing/runner.ts is removed, can we make runTests() internal?

Still a long way to that... That said, having removed runIfMain leaves only runTests as a way to trigger tests and I see no reason to disallow users to write their own test script that does some fancy stuff.

@nayeemrmn
Copy link
Collaborator

write their own test script that does some fancy stuff

Makes sense. If that's the purpose, though, we should make the exposed Deno.runTests() return result data and not do its own logging. That would make the current one unstable if not hidden...

@bartlomieju
Copy link
Member Author

write their own test script that does some fancy stuff

Makes sense. If that's the purpose, though, we should make the exposed Deno.runTests() return result data and not do its own logging. That would make the current one unstable if not hidden...

Sure, but let's discuss after we land this one.

CI still fails on glob expansion on Windows and I don't have a platform to test on 😕 I'm thinking about removing glob functionality completely for now

@bartlomieju bartlomieju changed the title [WIP] deno test refactor: rewrite deno test, add Deno.test() Feb 8, 2020
@bartlomieju bartlomieju requested a review from ry February 8, 2020 10:31
@nayeemrmn
Copy link
Collaborator

Yeah, I never realised how good globrex was. There's nothing that comes close in cargo.

For future: Do we have a way of shelling out small bits of work like this to JS? If not, we should. Then maybe we could bring globrex and the glob expander into cli/js 😆 at least until there is something in rustland.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Have you checked that if you insert a failing test into somewhere in cli/js that it will indeed still trigger failure? Same for std?

We need to be careful that we don’t silently ignore test failures when changing the runner.

}

/// Used for `deno test...` subcommand
const TEST_RUNNER_URL: &str = std_url!("testing/runner.ts");
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@bartlomieju
Copy link
Member Author

Have you checked that if you insert a failing test into somewhere in cli/js that it will indeed still trigger failure? Same for std?

We need to be careful that we don’t silently ignore test failures when changing the runner.

Certainly, both cases throw as expected. Keep in mind that this PR does not change testing in std/ - it only affects cli/js/. For std/ PR take a look at #3930

cli/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@ry ry 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 move forward with it - I think we need to get feedback. I'll try to cut a release today with it.

@bartlomieju
Copy link
Member Author

Great, once we land, I'll try to get PR with glob handling working

@ry
Copy link
Member

ry commented Feb 11, 2020

I think as long as it took a directory it would be fine (e.g. deno test .). I don't think that needs glob - you just walk the tree.

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