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

Test runner v2 #604

Merged
merged 29 commits into from Sep 28, 2019

Conversation

@nayeemrmn
Copy link
Contributor

commented Sep 18, 2019

Currently, a directory argument will match every file in that directory which would never be useful (and the way it's implemented is the cause of denoland/deno#2948). We only get to benefit from the default globs when no arguments are given, so they have to be written out manually unless you want the exact set of files matched by just deno test. See denoland/deno#2948 (comment).

This change will make it so that any directory given as - or expanded from - an argument is interpreted as a root on which to apply the default globs. Such an API would be more ergonomic and introduce some desirable properties. See #601 (comment).

The test runner will fail by default if no tests are found. Override this with a flag.

Other API changes:

//testing/runner.ts:
Add runTestModules() and RunTestModulesOptions, rename getMatchingUrls() to findTestModules().

//fs/mod.ts:
Rename RunOptions to RunTestsOptions.

//fs/glob.ts:
Add expandGlob(), expandGlobSync() and ExpandGlobOptions. These are prototypes extracted from the test runner.

//fs/path/constants.ts:
Add SEP.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

@nayeemrmn can you update the tests to present the new behavior of match? Also, I'm still convinced we should fail test run if no matching files are found (surely, a flag to disable this behavior can be added, but the default should fail), so if you could add that option as well.

testing/runner.ts Outdated 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
testing/runner.ts Outdated Show resolved Hide resolved
testing/runner.ts Outdated Show resolved Hide resolved
testing/runner.ts Show resolved Hide resolved
testing/runner.ts Show resolved Hide resolved
@nayeemrmn

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@bartlomieju @lucacasonato
Well, I introduced it. Didn't realise CI was using the live version of the test runner 😮 (Probably from when the runner was a 'tool' here and not part of the library. It should just be using deno test now IMO... that's for another issue). Now this is red obviously.

I suggest landing #599 first.

@nayeemrmn nayeemrmn force-pushed the nayeemrmn:test-runner branch from 7b83407 to 77feacc Sep 20, 2019
@nayeemrmn nayeemrmn marked this pull request as ready for review Sep 20, 2019
@nayeemrmn nayeemrmn changed the title Test runner v2 [WIP] Test runner v2 Sep 20, 2019
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:test-runner branch from 2237ce5 to d00887e Sep 20, 2019
@nayeemrmn nayeemrmn referenced this pull request Sep 20, 2019
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:test-runner branch 2 times, most recently from 40bd039 to 6bacb5f Sep 21, 2019
@nayeemrmn

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2019

@bartlomieju I think this is ready. Could I get another review?

cc @ry

@sholladay

This comment has been minimized.

Copy link

commented Sep 22, 2019

Is there a specific reason for having --allow-none? What's the use case? Seems crufty to me.

@nayeemrmn

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

Is there a specific reason for having --allow-none? What's the use case? Seems crufty to me.

@sholladay When the number of tests is dynamic somehow, or someone wants their infrastructure up before they have any tests? Maybe that's contrived 😅

While it makes sense practically, I just think that failing when no tests are run is 'illogical' enough that there should be a way of at least overriding it.

testing/runner.ts Outdated Show resolved Hide resolved
fs/glob.ts Show resolved Hide resolved
testing/runner.ts Show resolved Hide resolved
testing/runner.ts Show resolved Hide resolved
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:test-runner branch 12 times, most recently from 7567f35 to a494bec Sep 22, 2019
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:test-runner branch from 2db63ca to 6c1b4bf Sep 26, 2019
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:test-runner branch from 6c1b4bf to 31204c9 Sep 26, 2019
@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2019

@ry PTAL

xeval/test.ts Outdated Show resolved Hide resolved
testing/runner_test.ts Outdated Show resolved Hide resolved
@ry

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2019

Look like a nice clean up. Just a couple comments...

I'll land when @bartlomieju approves.

nayeemrmn added 2 commits Sep 27, 2019
@nayeemrmn

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2019

@bartlomieju Ready.

Copy link
Contributor

left a comment

LGTM, let's see this in action 💪

testing/runner.ts Outdated Show resolved Hide resolved
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:test-runner branch from c9ccba0 to bfb6423 Sep 28, 2019
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:test-runner branch from bfb6423 to b421efd Sep 28, 2019
@ry
ry approved these changes Sep 28, 2019
Copy link
Contributor

left a comment

LGTM - nice clean up

@ry ry merged commit 17a214b into denoland:master Sep 28, 2019
5 checks passed
5 checks passed
denoland.deno_std Build #20190928.7 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
@nayeemrmn nayeemrmn deleted the nayeemrmn:test-runner branch Sep 28, 2019
nayeemrmn added a commit to nayeemrmn/deno_std that referenced this pull request Oct 2, 2019
fs/glob.ts:
- Improve prototypes for expandGlob() and expandGlobSync() from denoland#604.
- Rename glob() to globToRegExp().
- Add normalizeGlob() and joinGlobs().
- Extract GlobToRegExpOptions from GlobOptions, remove the strict
  and filepath options.

fs/globrex.ts:
- Add GlobrexOptions.

fs/path/constants.ts:
- Add SEP_PATTERN.

fs/walk.ts:
- Add WalkOptions:includeFiles
- Default WalkOptions::includeDirs to true.
- Don't traverse directories matching a skip pattern.
- Remove walkSync()'s default root value.

prettier:
- Refactor to use expandGlob().

testing:
- Make findTestModules() an async generator.
nayeemrmn added a commit to nayeemrmn/deno_std that referenced this pull request Oct 2, 2019
fs/glob.ts:
- Improve prototypes for expandGlob() and expandGlobSync() from denoland#604.
- Rename glob() to globToRegExp().
- Add normalizeGlob() and joinGlobs().
- Extract GlobToRegExpOptions from GlobOptions, remove the strict
  and filepath options.

fs/globrex.ts:
- Add GlobrexOptions.

fs/path/constants.ts:
- Add SEP_PATTERN.

fs/walk.ts:
- Add WalkOptions::includeFiles
- Default WalkOptions::includeDirs to true.
- Don't traverse directories matching a skip pattern.
- Remove walkSync()'s default root value.

prettier:
- Refactor to use expandGlob().

testing:
- Make findTestModules() an async generator.
ry added a commit that referenced this pull request Oct 2, 2019
fs/glob.ts:
- Improve prototypes for expandGlob() and expandGlobSync() from #604.
- Rename glob() to globToRegExp().
- Add normalizeGlob() and joinGlobs().
- Extract GlobToRegExpOptions from GlobOptions, remove the strict
  and filepath options.

fs/globrex.ts:
- Add GlobrexOptions.

fs/path/constants.ts:
- Add SEP_PATTERN.

fs/walk.ts:
- Add WalkOptions::includeFiles
- Default WalkOptions::includeDirs to true.
- Don't traverse directories matching a skip pattern.
- Remove walkSync()'s default root value.

prettier:
- Refactor to use expandGlob().

testing:
- Make findTestModules() an async generator.
ry added a commit to ry/deno that referenced this pull request Oct 9, 2019
ry added a commit to ry/deno that referenced this pull request Oct 9, 2019
fs/glob.ts:
- Improve prototypes for expandGlob() and expandGlobSync() from denoland/deno_std#604.
- Rename glob() to globToRegExp().
- Add normalizeGlob() and joinGlobs().
- Extract GlobToRegExpOptions from GlobOptions, remove the strict
  and filepath options.

fs/globrex.ts:
- Add GlobrexOptions.

fs/path/constants.ts:
- Add SEP_PATTERN.

fs/walk.ts:
- Add WalkOptions::includeFiles
- Default WalkOptions::includeDirs to true.
- Don't traverse directories matching a skip pattern.
- Remove walkSync()'s default root value.

prettier:
- Refactor to use expandGlob().

testing:
- Make findTestModules() an async generator.
Original: denoland/deno_std@8c90bd9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.