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
Merged

Test runner v2 #604

merged 29 commits into from
Sep 28, 2019

Conversation

nayeemrmn
Copy link
Contributor

@nayeemrmn nayeemrmn 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
Copy link
Member

bartlomieju 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
Copy link
Contributor Author

@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 marked this pull request as ready for review September 20, 2019 17:11
@nayeemrmn nayeemrmn changed the title Test runner v2 [WIP] Test runner v2 Sep 20, 2019
@nayeemrmn nayeemrmn force-pushed the test-runner branch 2 times, most recently from 40bd039 to 6bacb5f Compare September 21, 2019 20:14
@nayeemrmn
Copy link
Contributor Author

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

cc @ry

@sholladay
Copy link

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

@nayeemrmn
Copy link
Contributor Author

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 test-runner branch 12 times, most recently from 7567f35 to a494bec Compare September 26, 2019 01:23
@bartlomieju
Copy link
Member

@ry PTAL

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

ry commented Sep 27, 2019

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

I'll land when @bartlomieju approves.

@nayeemrmn
Copy link
Contributor Author

@bartlomieju Ready.

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 see this in action 💪

testing/runner.ts 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 - nice clean up

@ry ry merged commit 17a214b into denoland:master Sep 28, 2019
@nayeemrmn nayeemrmn deleted the test-runner branch September 28, 2019 13:43
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 pushed 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 pushed a commit to ry/deno that referenced this pull request Oct 9, 2019
ry pushed a commit to ry/deno that referenced this pull request Oct 9, 2019
fs/glob.ts:
- Improve prototypes for expandGlob() and expandGlobSync() from denoland/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/std@8c90bd9
inverted-capital pushed a commit to dreamcatcher-tech/napps that referenced this pull request Oct 24, 2024
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.

5 participants