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

feat(cli): suggest matching commands #1763

Merged
merged 6 commits into from
Nov 26, 2019

Conversation

jamesgeorge007
Copy link
Contributor

closes #1762

@coveralls
Copy link

coveralls commented Nov 21, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 9d1b221 on jamesgeorge007:feat/suggest-commands into d17e6a8 on mozilla:master.

@Rob--W
Copy link
Member

Rob--W commented Nov 21, 2019

Thanks for the feature request and patch. Could you add a unit test to the following file?
https://github.com/mozilla/web-ext/blob/3c850ae49c59e6d56d7e6f189a873bb02ae30012/tests/unit/test.program.js

@Rob--W
Copy link
Member

Rob--W commented Nov 23, 2019

As you can see from the CI build output, the new test is failing: https://travis-ci.org/mozilla/web-ext/jobs/615041182

Please update the test to make sure that it passes.

@jamesgeorge007
Copy link
Contributor Author

@Rob--W can you give me a better picture regarding how it should be refactored as per the context?

@Rob--W
Copy link
Member

Rob--W commented Nov 25, 2019

I think that the relevant output is not an instance of UsageError. Try checking the program's output instead, like this:

.catch((error) => {
assert.match(error.message, /This command does not take any arguments/);
});

@Rob--W
Copy link
Member

Rob--W commented Nov 25, 2019

Could you revert the change to package-lock.json? The audit failure has been fixed on master, and if you rebase your patch on master, then that failure should go away.

The build still fails with the latest version of your patch. I took a closer look, and see why. Your unit test uses the Program constructor to run the program. But the recommendCommands() configuration is only defined in the main function. To test the desired behavior, you need to run the program instead of calling the constructor.

Here are examples:

Add a new test, e.g. with the file name tests/functional/test.typo.run.js

@Rob--W Rob--W self-requested a review November 25, 2019 17:14
@Rob--W
Copy link
Member

Rob--W commented Nov 26, 2019

Have you run the tests locally? The test is failing because the assert object has not been imported.

And the linting fails because the test imports symbols that it does not use.

Remove unused imports and import `assert to resolve these errors.

@Rob--W Rob--W merged commit 2d37dee into mozilla:master Nov 26, 2019
@Rob--W
Copy link
Member

Rob--W commented Nov 26, 2019

Thanks for the feature request and patch!

@jamesgeorge007 jamesgeorge007 deleted the feat/suggest-commands branch November 26, 2019 13:12
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.

[Feture Request]: suggest matching commands
3 participants