#2 More CLI Tests

[Romanx]

A pull for feedback as I work on more CLI tests on my fork

[Romanx]

@phated Just got some questions with some things as I was working on these few (of course feedback is appreciated!).

1. The require opts doesn't seem to do anything, I've worked out that it comes from liftoff but can't find any explict require of modules from the given path. Please let me know if I'm incorrect, I've chucked a placeholder test there for the moment to come back to.
2. The verify logic seems to work as intended however I expected by passing the --cwd flag that it would find the passed package.json (or named file) relative to that given path. It doesn't seem to be the case. Is my initial thinking incorrect or should it be relative to the given cwd? Until you let me know i've done a first pass making them relative made the default with a passed path (which I ideally don't want that test to do as I'm testing the default --verify works as intended)
3. My new test files have been based on the previously existing files however I seem to be getting linting errors with things that your existing files (which seem to be identical) don't have, any pointers on why that may be the case would be useful.

Let me know if there's anything glaring I'm doing wrong

[phated]

can you also update this in the cli-options.js? I totally forgot we accepted a path

[phated]

I'm really liking this PR. I'll answer your list:

1. The --require flag is passed to Liftoff at https://github.com/gulpjs/gulp-cli/blob/4.0/index.js#L77 which calls node's require on those modules before loading anything else. You could possibly test by making a local module that console.logs and then test that log is in the stdout output.
2. I think --cwd should be working as you stated. At https://github.com/gulpjs/gulp-cli/blob/4.0/index.js#L102 we are using configBase from Liftoff which is calculated using the cwd passed, but maybe one of our checks is off or something.
3. I believe I was excluding specific files from linting because they were just fixtures. It looks like the test/ directory isn't linted https://github.com/gulpjs/gulp-cli/blob/4.0/package.json#L56

[Romanx]

1. I'll create a simple module to test this functionality then, do you know how it gets into the gulpfile context at all?
2. configBase was set to null when i was debugging the cwd problem so I'll have a look at why it's not getting set.
3. Isn't jscs index.js lib bin test going to code check all of the test directory, that seems to be what's failing the travis build. Sorry if I'm getting confused easily, want to get it looking good :)

[Romanx]

I looked at the cwd problem. It seemed to be that it was using configBase as you suggested but that config base was being set to the path of the found gulpfile.js (in this case test/fixtures) as opposed to the passed cwd. After looking at the code in liftoff I found this so as the cwd (if undefined) was getting set the configBase which we were using anyway I updated the usage of verify to use the env.cwd instead as it will either be the passed setting or the default configBase. Let me know if you disagree with this change.

Another problem I spotted while down that rabbit hole was that by passing verify string arguments we can no longer use it as a bool flag from yargs unless i'm missing some docs from them. We can expect a value of "true" to be passed however this doesn't feel nice.

The only options I can see is forcing an argument to always be passed, have an empty string be the condition for using the default package.json or adding a separate flag like --verify-path which always takes an argument and have --verify as the default boolean case.

[phated]

@Romanx just wanted to post an update. I need to ask @tkellen what the difference between cwd and configBase is when coming from Liftoff before I decide on anything.

[tkellen]

Please see https://github.com/js-cli/js-liftoff/blob/master/README.md#optsconfigpath and let me know if that isn't clear?

[phated]

@tkellen it seems that I am looking for https://github.com/js-cli/js-liftoff/blob/master/README.md#callbackenv because configBase is in the resulting environment. This also harkens back to gulpjs/liftoff#55 as it seems that an explicit cwd should be resolved first, no?

[Romanx]

@phated Hey, i've updated the branch with tests for requiring a module which seems to work as expected.

I'm having problems testing --tasks-json as it's only available on the 4.0.0 branch and there is a switch internally on which operations can be performed based on the CLI version installed here. Because the gulpfile defines "gulp": ">=3.8.10" as a devDependency I can't test it as the cli will always find the latest 3.0 gulp and use that branch. (Which is good to see it working as intended).

Any suggestions on how to test it? It'd be a little silly to just remove the 3.8.10 package as then we can't test that case. I've seen this and we could perform some hackery by targeting gulpjs/gulp#4.0 with a name of gulp4.0 for example but then i'm not sure the switching would work.

I'll continue to work on others until I get a response

[phated]

@Romanx yeah, this is something we need to solve. I would ideally like to have integration tests for both code paths and have them both run on travis. I've been trying to think of the best solution... maybe we need to write a runner.js script that determines which tests to run

[phated]

@Romanx maybe you can just update that reference to gulpjs/gulp#4.0 to get these to pass for the time being

[Romanx]

@phated sounds good. I'll do that now and work on tasks-json at the same time.

[Romanx]

I'm considering not testing --color or --no-color as that would simply be testing supports-color works which has good test coverage. Any objections to this?

[Romanx]

@phated I found what was failing the linting, i'm on windows and it was adding window line endings to the file and the jscs config only considers unix line endings acceptable, fixed now.

I've finished the tasks-json by bumping to the 4.0 as a dev-dependency. This seems to work fine based on my knowledge of what the output is supposed to be however it will have broken other tests as gulp will now be trying to run ^3.7.0 gulpfiles which will fail.

Underneath writing to a path seems to use writeFileSync which is fine one I realised this was the case (from the error given) however it didn't work as I originally expected as all gulp outputs seem to write the path and create the file themselves. Perhaps this should do the same or we should document it in the readme? Until then i've added fs-extra to make it easier to create the output directory and tear it down after use. Hopefully this shouldn't cause issues.

There's a new things left to do but i think I've documented all the flags but here are the things left that i can see:

• Decide on a resolution for the configBase change I made above.
• Confirm that skipping color flags is okay as per comment above.

[Romanx]

@phated bump

[phated]

@Romanx sorry, I just gave a talk at Thunder Plains Conf so the past few days have been prepping that. I am still traveling but I plan to look at gulp issues tomorrow.

[Romanx]

@phated No problem just wanted to check you'd seen it!

[phated]

@Romanx the tests still seem to be failing on travis, can you take a look at that?

As for your list:

• Decide on a resolution for the configBase change I made above. - I'm good with your fix, the current behavior doesn't make sense
• Confirm that skipping color flags is okay as per comment above. Confirmed, we don't need to test those
  ** Decide on if we need more documentation for tasks-json flag path or decide to create path structure for the file writing. Need to think on this some more.
• Add switching of gulp-cli gulp dependency version based on tests running. That way we can test v4.0 and v3.8+ logic independently and fix the breaking tests. Let's do this one outside this PR

I will fully review everything once travis can complete successfully (I think it might just be updating some fixtures).

[Romanx]

@phated I've updated the fixtures that broke the tests due to the 4.0 upgrade. Had some issues with yargs options adding an extra space in the CI environment as opposed to my dev environment, took me a few commits and different methods of trying to normalise the outputs.

Hopefully you're happy with this, any changes you want just post and i'll get them done.

[phated]

Awesome work. Thanks!