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

Run tests over subfolders #53

Merged
merged 1 commit into from
Jul 27, 2016
Merged

Run tests over subfolders #53

merged 1 commit into from
Jul 27, 2016

Conversation

leobalter
Copy link
Contributor

@leobalter leobalter commented Jul 19, 2016

With this patch, test262-harness does not break passing folders or finding subfolders.

passing different paths will now work, where before only these forms were valid:

  • test/built-ins/Reflect/*.js
  • test/built-ins/Reflect/**/*.js

Now these other forms will be valid as well:

  • test/built-ins/Reflect/*
  • test/built-ins/Reflect/
  • test/built-ins/Reflect

There's a last commit part where I still want to improve, rather than just setting a try catch. While I don't know how to solve it yet, I'll leave this open to request for some feedback.

@leobalter
Copy link
Contributor Author

leobalter commented Jul 19, 2016

Plus, for clarification: setting a folder will run the tests through each subfolder, similarly the behavior of the deprecated test262 python runner.

Edit: a *.js is enough to avoid going recursively to subfolders.

if (fs.statSync(path).isDirectory()) {
path += '/**/*.js';
}
} catch(e) {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this try catch was necessary to avoid abruptions where the path is not expanded by default. This is perceived within the collateral tests. I'm not exactly sure how to make it safe before fs.statSync(path). Any known best practice?

@leobalter
Copy link
Contributor Author

ok, I've fixed the PR removing the files duplication. This change limited the possible valid paths, but the valid options are still an extension from the possibilities before this patch. This change limits the tool from diving recursively on subfolders, unless it's explicitly flagged as a glob pattern:

  • test/built-ins/Reflect/*: matches only the 3 .js files in that folder, skips the subfolders
  • test/built-ins/Reflect/**/*: matches those 3 files + all the files from subfolders
  • test/built-ins/Reflect: does not match any file
  • test/built-ins/Reflect/: does not match any file

These are now only considering npm-glob patterns, assuming the path arg needs to be wrapped under quotes on *nix systems cli usage to avoid path extension.


fileEvents.on('match', function (file) {
files.onNext(file);
if (fs.statSync(file).isFile()) {
Copy link
Owner

Choose a reason for hiding this comment

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

Glob has an option, "nodir", which will prevent it from matching directories. I think adding that option would do the same thing as this PR?

@leobalter leobalter force-pushed the glob branch 4 times, most recently from d9dde2a to fec20dd Compare July 27, 2016 21:59
@leobalter
Copy link
Contributor Author

at the end, this became just a small change. The nodir is working just fine.

@bterlson bterlson merged commit 120699b into bterlson:master Jul 27, 2016
@leobalter leobalter deleted the glob branch July 27, 2016 23:47
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.

None yet

2 participants