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

number of tests do not match up #271

Closed
tcurdt opened this issue Mar 7, 2016 · 42 comments
Closed

number of tests do not match up #271

tcurdt opened this issue Mar 7, 2016 · 42 comments

Comments

@tcurdt
Copy link

tcurdt commented Mar 7, 2016

tape -- tests/*.js core/tests/*.js plugins/*/tests/*.js
# tests 115

and

tape -- tests/*.js
# tests 34

tape -- core/tests/*.js
# tests 89

tape -- plugins/*/tests/*.js
# tests 81

34 + 89 + 81 = 204

115 != 204 WTF?

@ljharb
Copy link
Collaborator

ljharb commented Mar 7, 2016

Can you make a gist with the files in question?

@tcurdt
Copy link
Author

tcurdt commented Mar 7, 2016

I can do you one better - here is the full project https://github.com/tcurdt/xstatic

@tcurdt
Copy link
Author

tcurdt commented Mar 7, 2016

This is how my node_modules looks like

@tcurdt
babel-core
babel-plugin-transform-react-jsx
blue-tape
front-matter
handlebars
highlight.js
less
libxmljs
marked
minimatch
moment
node-sass
nunjucks
tape
xmlbuilder
xstatic-atom -> ../plugins/atom
xstatic-babel -> ../plugins/babel
xstatic-core -> ../core
xstatic-css -> ../plugins/css
xstatic-epub -> ../plugins/epub
xstatic-filter -> ../plugins/filter
xstatic-frontmatter -> ../plugins/frontmatter
xstatic-handlebars -> ../plugins/handlebars
xstatic-json -> ../plugins/json
xstatic-less -> ../plugins/less
xstatic-markdown -> ../plugins/markdown
xstatic-merge -> ../plugins/merge
xstatic-nunjucks -> ../plugins/nunjucks
xstatic-sass -> ../plugins/sass
xstatic-sitemap -> ../plugins/sitemap
xstatic-sort -> ../plugins/sort

@ljharb
Copy link
Collaborator

ljharb commented Mar 7, 2016

On a cloned repo, after npm install, for the first example, I get 204 tests run.

@tcurdt
Copy link
Author

tcurdt commented Mar 7, 2016

Hm. That's fine.

For development I've used npm link - could that lead to problems?

@ljharb
Copy link
Collaborator

ljharb commented Mar 7, 2016

Possibly - what are you linking exactly?

@tcurdt
Copy link
Author

tcurdt commented Mar 8, 2016

@ljharb see above - all the xstatic-* modules

@ljharb
Copy link
Collaborator

ljharb commented Mar 8, 2016

That absolutely could cause issues. npm link is meant to be used for separate modules - not to create symlinks within your primary module to avoid the perceived inconvenience of using "../"-based relative paths.

Closing this for now, since I can't reproduce - I don't think the tape runner needs to be following symlinks here.

@ljharb ljharb closed this as completed Mar 8, 2016
@tcurdt
Copy link
Author

tcurdt commented Mar 8, 2016

These are separate modules. I didn't use npm link to avoid "../" but to work on the latest version of modules (which is whattnpm link is meant for to my understanding). Not sure I can follow your argument there.

Reproducing this should be easy. Just rm -rf xstatic-<you pick one> and then use npm link to set it to the latest version.

@ljharb
Copy link
Collaborator

ljharb commented Mar 8, 2016

@tcurdt i may have misunderstood - you said you have xstatic-atom -> ../plugins/atom for example, and the implication was you're running tests inside plugins/atom from the same project that xstatic-atom is linked in?

@tcurdt
Copy link
Author

tcurdt commented Mar 8, 2016

@ljharb since the xstatic project is early stage I have all plugins and the core in one repo (like react and ember are doing it as well). The core and the plugins are all individual modules though (with own package.json files).

At the top level I am running some integration tests (which basically just another project linking to all the other modules). So best to forget all is in the same repo - that doesn't really matter. The ../ is merely a coincident.

tape -- tests/*.js core/tests/*.js plugins/*/tests/*.js | faucet

is run on the top level. Which seems to work OK with a fresh install. When I use npm link though (to use the latest versions for development) I am seeing the reported behaviour.

Does that make more sense now?

@ljharb
Copy link
Collaborator

ljharb commented Mar 8, 2016

Hmm. It's likely that the core/tests/*.js are being skipped - if you run tape -- tests/*.js plugins/*/tests/*.js | faucet do you still get the same 115 tests?

@tcurdt
Copy link
Author

tcurdt commented Mar 8, 2016

Yes, tape -- tests/*.js plugins/*/tests/*.js | faucet gives me 115 tests.

@ljharb
Copy link
Collaborator

ljharb commented Mar 8, 2016

OK - then since core itself is a symlink (and plugins/foo is, but plugins is not) i suspect it's that the tape runner doesn't traverse symlinks. if you put core inside a dir, my hunch is that you'd get all 204. @tcurdt, can you confirm?

@tcurdt
Copy link
Author

tcurdt commented Mar 11, 2016

Not sure I can follow. You think foo/core should work?

@ljharb
Copy link
Collaborator

ljharb commented Mar 12, 2016

@tcurdt i think that if the first "item" in one of the paths you pass to tape is a symlink, it will break, but otherwise, it will work

@tcurdt
Copy link
Author

tcurdt commented Mar 12, 2016

@ljharb but none of those are symlinks - the symlinks are only in node_modules

But I am seeing even more strangeness.

tape -- plugins/sass/tests/*.js

works fine when called like above, now when it's part of match

tape -- plugins/*/tests/*.js

it fails.

WTF? Any idea?
(This may or may not be related)

@ljharb
Copy link
Collaborator

ljharb commented Mar 12, 2016

I'm going to reopen this for now - it would help if you made a branch on the repo that had the symlinks committed to it was easier to repro.

@ljharb ljharb reopened this Mar 12, 2016
@tcurdt
Copy link
Author

tcurdt commented Mar 12, 2016

This seems to be an unrelated issue - but try the following:

git clone git@github.com:tcurdt/xstatic.git && cd xstatic
git checkout tc-testing
npm i
sh deps.sh

then try (1)

tape -- plugins/sass/tests/*.js

and then (2)

tape -- plugins/*/tests/*.js

You will find that the test worked in (1) but fails in (2).

I'll try to make the linking easily reproducible as well.

@tcurdt
Copy link
Author

tcurdt commented Mar 12, 2016

If you run links.sh it will create the setup I had.

tape -- core/tests/*.js # tests 89
tape -- plugins/*/tests/*.js # tests 81
tape -- tests/*.js # tests 34
tape -- tests/*.js core/tests/*.js plugins/*/tests/*.js # tests 115
tape -- core/tests/*.js plugins/*/tests/*.js tests/*.js # tests 89

@ljharb
Copy link
Collaborator

ljharb commented Mar 13, 2016

wow, lots of links :-) k, i've reproduced it locally - thanks!

I also see:

tape -- plugins/*/tests/*.js tests/*.js core/tests/*.js # tests 115
tape -- plugins/*/tests/*.js core/tests/*.js tests/*.js # tests 115
tape -- plugins/*/tests/*.js tests/*.js # tests 115
tape -- tests/*.js plugins/*/tests/*.js # tests 115
tape -- core/tests/*.js tests/*.js plugins/*/tests/*.js # tests 89
tape -- core/tests/*.js tests/*.js # tests 89
tape -- core/tests/*.js plugins/*/tests/*.js # tests 89
tape -- tests/*.js core/tests/*.js # tests 34
tape -- tests/{behaviour-nop,it-blog}.js core/tests/{changes,collection,context,glob,lazy,project}.js # tests 34
tape -- tests/it-blog.js core/tests/*.js # tests 30
tape -- tests/behaviour-nop.js core/tests/{changes,collection,context,glob,lazy,project}.js # tests 84
tape -- tests/behaviour-nop.js core/tests/{changes,collection,context,glob,lazy,project}.js # tests 4 (but i see results from 84)

very weird. I'm not sure how to solve this nor why it would be happening. The fact that it requires all those links suggests it's a symlink traversal problem.

@tcurdt
Copy link
Author

tcurdt commented Mar 15, 2016

I now have all the tests running from the parent project. For head development I use a script that replaces the deps with straight symlinks. That works around the problem (and is much faster).

The original problem is mega odd though.
I am wondering if this could be related to the use of blue-tape.

@ljharb
Copy link
Collaborator

ljharb commented Mar 15, 2016

That is possible. it would be useful to be able to repro it without blue-tape.

@brendanlong
Copy link
Contributor

Do you need to use glob's "follow" option?

@ljharb
Copy link
Collaborator

ljharb commented Mar 25, 2016

I tried that, but it didn't seem to fix the problem.

@LongLiveCHIEF
Copy link

LongLiveCHIEF commented May 8, 2016

I've just found that I'm unable to test more than one dir using glob expansion, and I think it's another indicator of this issue.

For example:

tape ./test/**/*.spec.js is the command I'm running, against:

-/test
|- example.spec.js
|---/core
       |-componentA.spec.js
       |-componentB.spec.js
       |--/sub-core
             |- componentC.spec.js

Only tests from the /core folder however, were run.

I created a test glob.js file, and gave it the same ./test/**/*.spec.js pattern, and it logged out all 4 files.

ar glob = require("glob");

glob("./test/**/*.spec.js", function(er, files){

    files.forEach(function(file){
        process.stdout.write(`matched file: ${file}\r\n`);  
    });
});

Here's the commit from my project demoing this.

I think the problem has to be related to how the results from glob are passed to tap?

@LongLiveCHIEF
Copy link

LongLiveCHIEF commented May 22, 2016

I think the problem is in the bin/tape file. I don't think the glob is correctly assigned to opts._ when any cli option is given, or whenever -- is used, in combination with being run from an npm-script.

@LongLiveCHIEF
Copy link

I'm going to create a test/bin.js file, and see if I can write some test conditions that should pass, but fail, and then go from there. I suspect that we'll need to add {--:true} to the parser.

@LongLiveCHIEF
Copy link

Ok, definitely some odd things going on here, and I think it's because minimist is parsing the globs. I created the following changes to the bin/tape file in my LongLiveCHIEF/tape fix-271 branch, in order to first get an idea of what exactly is being parsed into opts._.

I expected the globs to be passed in, however, in each case, the glob's have already been matched. If you want, you can pull the changes in my fork into your local clone of substack/tape, then run npm link from within your tape module.

Then, you can go to any other project you are running tests using tape, and run npm link tape. Now when you execute tape, it will create a test-matches.txt file in the cwd.

As I continue to look into this, I wanted you guys to be able to look at what I'm seeing with these early findings. Very interesting!

@LongLiveCHIEF
Copy link

Here's another quirk. I'm running:

tape -r babel-register -r ./test/common.js ./test/**/*.spec.js

When I run this using /bin/bash, or execute using npm test, it matches only 2 of the files given in my example above. However, when I use /bin/zsh, it matches all 4 correctly.

@LongLiveCHIEF
Copy link

@tcurdt @ljharb time to let me know about your environment conditions:

  • What shell were you using?
  • What version of node are you running?
  • Were you running the tape command via npm test?

@LongLiveCHIEF
Copy link

Looks like node itself is expanding the glob's as they're assigned to process.argv. If you put the glob in a string, everything works perfectly:

Bad:
tape ./test/*.js

Good:
tape './test/*.js'

@tcurdt
Copy link
Author

tcurdt commented May 22, 2016

Interesting findings, @LongLiveCHIEF

  1. shell: GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin15)
  2. node: was 4.3.x or 4.4.x
  3. I have it setup for npm but I tried both ways

@ljharb
Copy link
Collaborator

ljharb commented May 22, 2016

@LongLiveCHIEF putting the globs in quotes is always a requirement, or else the shell will expand it for you. This isn't unique to tape, or glob, but is part of any command line tool.

@tcurdt does everything work for you if you pass the file path in quotes?

@LongLiveCHIEF
Copy link

LongLiveCHIEF commented May 22, 2016

Bug Confirmed

Cause

This bug occurs depending on whether or not your shell supports recursive globbing. By the time the values in process.argv are able to be parsed, the glob's passed have already been expanded into matches files, which is done by your shell.

Resolution Options

  1. create a subproc and pipe everything after tape in the node process call into stdin for the subproc (as a single string), and then parse that with minimist instead.
  2. Just update all the usage docs to ensure the glob pattern uses quotes around the glob to be matched.

Temporary Workaround

In the short term, you should just be able to encase your glob's in strings in order to get the right number of tests to run. Examples:

From the command line:

$ tape './test/**/*.js'

Or using npm:

// package.json
{
    "scripts":{
        "test": 'tape "./test**/*.js"'
    }
}

// run using `npm test`

(/cc @tcurdt )

Input Needed

I'd like a project owner/maintainer to chime in on what direction they'd prefer so I know which route to go with my PR.

I checked out mocha, and it looks like they get the usability without the string encasing by going with the first option.

(/cc @ljharb )

@ljharb
Copy link
Collaborator

ljharb commented May 22, 2016

Certainly option 2 seems simplest, and is what most command line tools require.

However, I'd love to see what a PR would look like for option 1 (even though I think we should always recommend quoting path arguments in all cases).

@LongLiveCHIEF
Copy link

LongLiveCHIEF commented May 22, 2016

I think it would add about 40-60 loc to the /bin directory, but it would be a non-breaking change.

I think option 2 is certainly easier. I thought it was fairly standard practice to support globbing without string encasing, but after digging around a bit, it's 50/50.

If we go with option 2, I think we should update the README to mimic the [eslint.org cli-usage docs](http://eslint.org/docs/user-guide/command-line-interface homepage, up through the part where it says:

Please note that when passing a glob as a parameter, it will be expanded by your shell. The results of the expansion can vary depending on your shell, and its configuration. If you want to use node glob syntax, you have to quote your parameter (using double quotes if you need it to run in Windows), as follows

@LongLiveCHIEF
Copy link

Looks like all this time, the Usage section of the README.md had us covered.

@tcurdt can you confirm that encasing your glob's as strings solves your problem, and then we can close?

@tcurdt
Copy link
Author

tcurdt commented May 22, 2016

@ljharb I changed the setup and get by without the linking - and with that all works as is.
@LongLiveCHIEF Frankly speaking - I don't get it. If quoting is the problem/solution - why the different counts? The expansion should be the same for the combined and the three separate calls to tape.

@LongLiveCHIEF
Copy link

Can I ask... why are you using -- in your command execution?

@ljharb ljharb removed the bug label May 22, 2016
@tcurdt
Copy link
Author

tcurdt commented May 22, 2016

@LongLiveCHIEF could that be a problem?

I guess that's a leftover from the npm setup where I use it with istanbul

istanbul test tape -- tests/*.js core/tests/*.js plugins/*/tests/*.js | faucet

@ljharb
Copy link
Collaborator

ljharb commented May 22, 2016

I don't think that should be a problem. However, they definitely need to be quoted.

I think I'm going to close this for now.

If someone wants to try a PR that makes unquoted options work, that'd be very interesting to consider.

@ljharb ljharb closed this as completed May 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants