Test bin command #1980

hpurmann · 2016-11-22T11:13:59Z

Summary
This PR is a response to #1804

In the first step, I added a test for the existing behaviour. I was about to fix the bug @shnhrrsn reported. But then I noticed that none of yarns commands work in subdirectories. This is certainly a difference compared to npm. Is this intended?

We could add logic to find the current package root. I did so locally by traversing the folders up and searching for a package.json, then taking this as the cwd. It would add some complexity which could lead to other bugs. What do you think?

And a minor thing: Do we want to tell the user when he uses too many arguments (like we do in yarn why) or do we want be consistent with npm bin and just work, no matter how many arguments got passed to bin?

hpurmann · 2016-11-24T19:43:05Z

@torifat, @kittens, any thoughts on being able to execute yarn in sub-directories?

sebmck · 2016-11-25T14:05:09Z

src/cli/commands/bin.js

@@ -19,6 +19,6 @@ export function run(
    config.registries[RegistryYarn.registry].folder,
    '.bin',
  );
-  console.log(binFolder);
+  reporter.log(binFolder);


This will output an ansi code at the beginning of the line that resets the cursor to the start making it unsuitable for consumption via another tool.

Oh ok, interesting. Is there a good way to test the console output of this command without using the reporter?

sebmck · 2016-11-25T14:06:20Z

__tests__/commands/bin.js

+
+const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'bin');
+
+async function runBin(


Can you use the helper method that I cleaned up in #2007?

Ok sure, had a pretty similar thought about this boilerplate code.

bestander · 2017-02-28T12:28:30Z

@hpurmann, what is left to get this merged?
I think it would be a good start to get this feature right.

hpurmann · 2017-02-28T15:12:21Z

Thanks for having a look at this.

Actually a few things are missing here:

I need to use the helper method @kittens cleaned up in DRY up command tests #2007
I need to know how to test the output of the command if I'm not allowed to use the reporter (see comments) For example I could spy on the console
Do you want to only have the test or do we want to also change the behaviour (that yarn bin works in sub-folders)? Currently, most of yarns commands do not work in sub-folders. This should maybe be first discussed as an RFC

bestander · 2017-02-28T19:46:09Z

Let's spy on console for simplicity.
An alternative would be to use a reporter that does only console.
Let's start with a test, merge it and move on to the fix discussion

Sinon provides an easy way to spy on objects and to clean up after each test case.

The problem here is that I can't speficy a directory from where I'd like to simulate calling `bin`. Therefore, a test like that doesn't work.

hpurmann · 2017-03-01T00:27:54Z

I use the helper method now but got a problem with it (see below in 3). Maybe @kittens or you @bestander can help me with it?
I spy on console and use sinon to facilitate resetting the spy after each test case and cleaning it up in the end.
I still pushed a skipped test for the sub directory thing which makes clear that my current approach for these tests is not working. Ideally, I want to tell yarn to execute the bin command from a specified directory (the fixtureLoc). Then, in the test expectation, I want to check whether it told me the correct ./node_modules/.bin path in that directory. Unfortunately, the test does create a temporary directory by concatenating the random path and the specified name (e.g. "subdir"). So if I'm using this information generated by the test, I do not actually test anything.

bestander · 2017-03-03T16:39:44Z

package.json

@@ -71,6 +71,7 @@
    "gulp-watch": "^4.3.5",
    "jest": "^19.0.2",
    "mock-stdin": "^0.3.0",
+    "sinon": "^1.17.7",


I think jest can spy on its own without sinon

Ok, didn't know this. I'll change this. Can you give me a hint how to handle the problem described in 3.?

Re 3, maybe you should not test for Path but rather test that a script from bin can be executed?

bestander · 2017-04-06T15:28:52Z

@hpurmann, are you blocked by something on this PR?

hpurmann · 2017-04-20T15:37:50Z

No, currently just missing time. Will get back to it.

bestander · 2017-05-12T19:43:57Z

Hey, @hpurmann, your contribution is very much welcome.
Once you have a chance to work on it open a new PR.
I'll close it for now to not keep unactionable PRs open.

hpurmann changed the title ~~[WIP] Test bin command, make yarn look for package root?~~ [RFC] Test bin command, make yarn look for package root? Nov 24, 2016

sebmck reviewed Nov 25, 2016

View reviewed changes

hpurmann mentioned this pull request Feb 24, 2017

Root package.json is not used #2693

Closed

hpurmann added 4 commits February 28, 2017 23:32

Add test for bin command

2294abb

Add sinon

c529fd1

Sinon provides an easy way to spy on objects and to clean up after each test case.

Adapt test to use the buildRun helper method

922e63e

Add skipped test case for sub directories

22d0f89

The problem here is that I can't speficy a directory from where I'd like to simulate calling `bin`. Therefore, a test like that doesn't work.

hpurmann changed the title ~~[RFC] Test bin command, make yarn look for package root?~~ Test bin command Mar 1, 2017

bestander reviewed Mar 3, 2017

View reviewed changes

bestander mentioned this pull request Mar 10, 2017

Log the binFolder result from the bin command via the reporter. #2844

Closed

bestander closed this May 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test bin command #1980

Test bin command #1980

hpurmann commented Nov 22, 2016 •

edited

Loading

hpurmann commented Nov 24, 2016

sebmck Nov 25, 2016

hpurmann Nov 25, 2016

sebmck Nov 25, 2016

hpurmann Nov 25, 2016 •

edited

Loading

bestander commented Feb 28, 2017

hpurmann commented Feb 28, 2017 •

edited

Loading

bestander commented Feb 28, 2017

hpurmann commented Mar 1, 2017

bestander Mar 3, 2017

hpurmann Mar 4, 2017

bestander Mar 6, 2017

bestander commented Apr 6, 2017

hpurmann commented Apr 20, 2017

bestander commented May 12, 2017


		const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'bin');

		async function runBin(

Test bin command #1980

Test bin command #1980

Conversation

hpurmann commented Nov 22, 2016 • edited Loading

hpurmann commented Nov 24, 2016

sebmck Nov 25, 2016

Choose a reason for hiding this comment

hpurmann Nov 25, 2016

Choose a reason for hiding this comment

sebmck Nov 25, 2016

Choose a reason for hiding this comment

hpurmann Nov 25, 2016 • edited Loading

Choose a reason for hiding this comment

bestander commented Feb 28, 2017

hpurmann commented Feb 28, 2017 • edited Loading

bestander commented Feb 28, 2017

hpurmann commented Mar 1, 2017

bestander Mar 3, 2017

Choose a reason for hiding this comment

hpurmann Mar 4, 2017

Choose a reason for hiding this comment

bestander Mar 6, 2017

Choose a reason for hiding this comment

bestander commented Apr 6, 2017

hpurmann commented Apr 20, 2017

bestander commented May 12, 2017

hpurmann commented Nov 22, 2016 •

edited

Loading

hpurmann Nov 25, 2016 •

edited

Loading

hpurmann commented Feb 28, 2017 •

edited

Loading