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: test runner #516

Merged
merged 20 commits into from Aug 15, 2019

Conversation

@bartlomieju
Copy link
Contributor

commented Jun 22, 2019

This PR closes #193

It provides simple test runner that searches recursively for *_test.js and *_test.ts files.

I'd prefer to land it ASAP in simplest version and then start bike-shedding based on our needs.

Waiting on #556 to land

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-test_runner branch from 423d81a to 40c903a Aug 9, 2019

@bartlomieju bartlomieju changed the title WIP feat: test runner feat: test runner Aug 12, 2019

testing/runner.ts Outdated Show resolved Hide resolved
testing/runner.ts Outdated Show resolved Hide resolved

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-test_runner branch from e7f4072 to f4c8fc3 Aug 14, 2019

bartlomieju added 2 commits Aug 14, 2019
testing/runner.ts Outdated Show resolved Hide resolved
@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Alright so we need to settle on a couple of things for the MVP:

  1. Default globs/file pattern. Currently there are only two default globs:
**/*_test.ts,
**/*_test.js

I agree with @hayd that we should also match:

**/test.ts,
**/test.js,
  1. CLI
    There are at least a few options that we need to support for MVP, namely:
  • --quiet - to capture console.logs
  • --failfail - stop on first failure
  • --exclude - exclude globs for file matching - in deno_std we need to exclude node_modules/ otherwise we'd be loading test files from there as well
  • [globs...] - include globs for file matching

For now let's skip these options:

  • parallel - not really working as expected with test runner, it just run all Promises simultaneously, flaky in my experience
  • only/skip - run only tests matching/not matching specific name - I want to refactor testing framework a bit, including adding test.only and test.skip functions (see #214), that would produce problems down the road

CC @ry @hayd

bartlomieju added 2 commits Aug 14, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-test_runner branch from ace44af to 059f083 Aug 14, 2019

@ry

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Is //test.ts dead code now? Remove that.

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Is //test.ts dead code now? Remove that.

Yes. I removed all not needed anymore files. Although in test.ts we had this checkSourceFileChanges function which I'm not yet sure how to coordinate with runner.

I guess extracting it to separate file and passing datetime as arg could work.

@j-f1
Copy link

left a comment

For reference, here are the equivalent options from Jest:

deno -A https://deno.land/std/testing/runner.ts [OPTIONS] [FILES...]
OPTIONS:
-q, --quiet Don't show output from test cases

This comment has been minimized.

Copy link
@j-f1

j-f1 Aug 14, 2019

--silent

Prevent tests from printing messages through the console.

OPTIONS:
-q, --quiet Don't show output from test cases
-f, --failfast Stop test suite on first error

This comment has been minimized.

Copy link
@j-f1

j-f1 Aug 14, 2019

--bail

Alias: -b. Exit the test suite immediately upon n number of failing test suite. Defaults to 1.

-q, --quiet Don't show output from test cases
-f, --failfast Stop test suite on first error
-e, --exclude <FILES...> List of file names to exclude from run. If this options is
used files to match must be specified after "--".

This comment has been minimized.

Copy link
@j-f1

j-f1 Aug 14, 2019

--testPathIgnorePatterns=[array]

An array of regexp pattern strings that is tested against all tests paths before executing the test. Contrary to --testPathPattern, it will only run those test with a path that does not match with the provided regexp expressions.

This comment has been minimized.

Copy link
@j-f1

j-f1 Aug 14, 2019

Also, I’m wondering if -x would be a better short name for this option if you keep the exclude name.

@j-f1

This comment has been minimized.

Copy link

commented Aug 14, 2019

Although in test.ts we had this checkSourceFileChanges function which I'm not yet sure how to coordinate with runner.

I guess extracting it to separate file and passing datetime as arg could work.

Maybe the test runner should have that as a feature/option?

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Maybe the test runner should have that as a feature/option?

That's specific action to this repo. Test runner should be agnostic of such things

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-test_runner branch from f9fdcd7 to a892063 Aug 14, 2019

testing/runner.ts Show resolved Hide resolved
testing/runner.ts Outdated Show resolved Hide resolved
testing/runner.ts Outdated Show resolved Hide resolved
testing/runner.ts Outdated Show resolved Hide resolved

@ry ry requested a review from piscisaureus Aug 14, 2019

bartlomieju added 4 commits Aug 14, 2019

// TODO: change return type to `Promise<void>` once, `runTests` is updated
// to return boolean instead of exiting
export async function main(root: string = cwd()): Promise<void> {

This comment has been minimized.

Copy link
@ry

ry Aug 14, 2019

Contributor

Describe the glob behavior in a jsdoc comment

bartlomieju added 2 commits Aug 14, 2019
@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

CI now is considerably slower (2:10 on latest master and 3:47 on this branch)

@piscisaureus
Copy link
Contributor

left a comment

Yes, great!

@ry

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

@bartlomieju @piscisaureus did you confirm that breaking some random test still breaks the runner?

@ry

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Slowness is due to the fact that the TS compiler worker is starting up and shutting down for each test file.

@ry
ry approved these changes Aug 15, 2019
Copy link
Contributor

left a comment

LGTM - this is great to have. I confirmed that adding a failing test breaks it.

The test runner is quite slow now. This clearly shows that our TS compiler (cli/compiler/ts.rs) is suboptimal in some way. Let's fix it in core tho.

@ry ry merged commit c44e536 into denoland:master Aug 15, 2019

5 checks passed

denoland.deno_std Build #20190814.18 succeeded
Details
denoland.deno_std (Linux) Linux succeeded
Details
denoland.deno_std (Mac) Mac succeeded
Details
denoland.deno_std (Windows) Windows succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@bartlomieju bartlomieju deleted the bartlomieju:feat-test_runner branch Aug 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.