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
Merged

feat: test runner #516

merged 20 commits into from
Aug 15, 2019

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju 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 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
testing/runner.ts Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member Author

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 Discussion: run specific test - test.only #214), that would produce problems down the road

CC @ry @hayd

@ry
Copy link
Member

ry commented Aug 14, 2019

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

@bartlomieju
Copy link
Member Author

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.

Copy link

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

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
Copy link

Choose a reason for hiding this comment

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

--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
Copy link

Choose a reason for hiding this comment

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

--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 "--".
Copy link

Choose a reason for hiding this comment

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

--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.

Copy link

Choose a reason for hiding this comment

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

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

@j-f1
Copy link

j-f1 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
Copy link
Member Author

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

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 August 14, 2019 19:13

// 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> {
Copy link
Member

Choose a reason for hiding this comment

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

Describe the glob behavior in a jsdoc comment

@bartlomieju
Copy link
Member Author

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

Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

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

Yes, great!

@ry
Copy link
Member

ry commented Aug 14, 2019

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

@ry
Copy link
Member

ry 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.

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 - 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
@bartlomieju bartlomieju deleted the feat-test_runner branch August 15, 2019 13:20
ry pushed a commit to ry/deno that referenced this pull request Oct 9, 2019
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.

Provide a test runner
6 participants