chore(cli): refactor checks into decorators #121

louis-bompart · 2021-03-30T03:16:50Z

Split the Node and NPX checks from the React command and into preconditions
Refactor the Authentication decorator into a precondition
Added a Preconditions checker decorator.

Unit Tested 💯

CDX-203

github-actions · 2021-03-30T03:17:26Z

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

louis-bompart · 2021-03-30T04:15:39Z

packages/cli/package.json

@@ -19,6 +19,7 @@
    "cli-ux": "^5.5.1",
    "coveo.analytics": "^2.18.4",
    "create-react-app": "^4.0.3",
+    "dedent": "^0.7.0",


Neat library to avoid oopsies in indentation with template string
https://www.npmjs.com/package/dedent

louis-bompart · 2021-03-30T04:16:14Z

packages/cli/package.json

    "@types/fs-extra": "^9.0.6",
+    "@types/jest": "^26.0.22",


To have describe in here.

louis-bompart · 2021-03-30T04:20:09Z

packages/cli/package.json

    "@types/node": "^10",
    "chai": "^4.2.0",
    "jest": "^26.6.3",
    "patch-package": "^6.2.2",
    "rimraf": "^3.0.2",
    "ts-jest": "^26.5.1",
    "ts-node": "^8",
-    "typescript": "^3.3"
+    "typescript": "4.2.3"


Could be reverted:
I wanted to migrate the tsconfig to a composite project to split the test tsconfig from the source tsconfig (to avoid having describe everywhere), but was unable because of oclif/oclif#299.
Oclif went hay-wire when I tried to split it.

louis-bompart · 2021-03-30T04:21:13Z

packages/cli/src/commands/auth/login.spec.ts

@@ -75,12 +75,12 @@ describe('auth:login', () => {
  test
    .command(['auth:login', '-e', 'foo'])
    .catch(/Expected --environment=foo/)
-    .it('reject invalid environment');
+    .it('reject invalid environment', async () => {});


I noted timing issue and promise timing out here.
I'm honestly not too sure of what's going in in this tests, the syntax is really painful to read.

the syntax is really painful to read.

What it does:

* start a test * execute the command auth:login -e foo -> try to login in environment "foo" which is not a valid environment * catch the error, and regex match on `/Expected --environment=foo/` on the output (stderr) * .it(the description of the test)

https://oclif.io/docs/testing and https://github.com/oclif/fancy-test for more documentation

That's how all the UT for the commands themselves will look like, so better get used to it ;)

louis-bompart · 2021-03-30T04:22:13Z

packages/cli/src/commands/ui/create/react.ts


-  static description = `Create a Coveo Headless-powered search page with the React web framework. See ${linkToReadme}`;
+  static description =


Because the page about the command was really shallow after reworking it, I removed the link.

louis-bompart · 2021-03-30T04:23:47Z

packages/cli/src/hooks/init/set-global-config.ts

-declare global {
-  namespace NodeJS {
-    interface Global {
-      config: IConfig;
-    }
-  }
-}


I put the typing shenanigan in a declaration file to keep things tidy.

louis-bompart · 2021-03-30T04:25:15Z

packages/cli/src/lib/decorators/preconditions/node.spec.ts

+      expect(fakeCommand.warn).toHaveBeenCalledWith(dedent`
+        Required version invalid: "foo".
+        Please report this error to Coveo: https://github.com/coveo/cli/issues/new
+      `);


See below for more explanation ;)

louis-bompart · 2021-03-30T04:26:29Z

packages/cli/src/lib/decorators/preconditions/node.spec.ts

+      };
+
+      await expect(
+        IsNodeVersionAbove('0.0.1')((fakeCommand as unknown) as Command)


I prolly could do a bit cleaner than the double cast with some Jest-pirouette, but I didn't think it was worth the hassle.

louis-bompart · 2021-03-30T04:29:25Z

packages/cli/src/lib/decorators/preconditions/node.ts

+      target.warn(dedent`
+        Required version invalid: "${requiredVersion}".
+        Please report this error to Coveo: https://github.com/coveo/cli/issues/new
+      `);


If this warning is triggered, there's a big chance that it's not the user machine that ain't properly set up, but that we messed up and didn't call properly the function.

Hopefully, this code will never run, but if it does, at least the user will know what to do (contact us so we can fix it).

louis-bompart · 2021-03-30T04:30:17Z

packages/cli/src/lib/decorators/preconditions/node.ts

+export function warnHowToInstallNode(target: Command) {
+  target.warn(dedent`
+    Please visit ${linkToReadme} for more detailed installation information.
+  `);
+}
+
+export function isMissingExecutable(output: SpawnProcessOutput) {
+  return (
+    output.exitCode !== null &&
+    Math.abs(output.exitCode) === constants.errno.ENOENT
+  );
+}


Will be refactorized on a subsequent PR as utils, for now, that was not worth it. (when introducing decorator for Angular).

louis-bompart · 2021-03-30T04:31:58Z

packages/cli/src/lib/decorators/preconditions/preconditions.ts

+) => Boolean | Promise<Boolean>;
+
+export function Preconditions(...preconditions: PreconditionFunction[]) {
+  return function (


Fun anecdote: don't put async here, typings get messy (rightfully so)

y-lakhdar

Nice Job

olamothe · 2021-03-30T14:40:43Z

packages/cli/src/commands/auth/login.spec.ts

@@ -75,12 +75,12 @@ describe('auth:login', () => {
  test
    .command(['auth:login', '-e', 'foo'])
    .catch(/Expected --environment=foo/)
-    .it('reject invalid environment');
+    .it('reject invalid environment', async () => {});


the syntax is really painful to read.

What it does:

* start a test * execute the command auth:login -e foo -> try to login in environment "foo" which is not a valid environment * catch the error, and regex match on `/Expected --environment=foo/` on the output (stderr) * .it(the description of the test)

https://oclif.io/docs/testing and https://github.com/oclif/fancy-test for more documentation

That's how all the UT for the commands themselves will look like, so better get used to it ;)

olamothe · 2021-03-30T14:52:08Z

packages/cli/src/lib/decorators/preconditions/preconditions.spec.ts

+  describe('if all preconditions succeed', () => {
+    it('should executes preconditions the original command', async () => {
+      let counter = 0;
+      const orderChecker = jest.fn();


I see you put quite a lot of emphasis in the tests on "the order is respected".

Is that really an important/meaningful thing to test ?

I guess it can be, if we have preconditions that depends on one another, but right now it does not seem to be the case that we have such a need.

The most important thing to verify is that the original function is only called after all preconditions had a chance to execute.

Maybe we could simplify the test to only focus on this type of verification ?

olamothe · 2021-03-30T14:54:00Z

packages/cli/src/lib/decorators/preconditions/preconditions.spec.ts

+  });
+
+  describe('if one precondition failed', () => {
+    it('should executes all the preconditions prior to the failing one and nothing else', async () => {


Similar comment as above, is that really something that we are interested in ?

If all preconditions are independant from one another, the order is not actually important, just that the "command" does not get executed if at least one precondition fails..

olamothe · 2021-03-30T14:57:31Z

packages/cli/src/lib/decorators/preconditions/npx.spec.ts

+      };
+
+      await expect(
+        IsNpxInstalled()((fakeCommand as unknown) as Command)


For readability's sake, I would add a function getFakeCommand(): Command {...} util function that does the cast + boilerplate.

That you could reuse for (I think?) all the tests in this file.

olamothe · 2021-03-30T15:02:45Z

packages/cli/src/lib/decorators/preconditions/npx.spec.ts

+        IsNpxInstalled()((fakeCommand as unknown) as Command)
+      ).resolves.toBe(false);
+      expect(fakeCommand.warn).toHaveBeenCalledTimes(2);
+      expect(fakeCommand.warn).toHaveBeenCalledWith(dedent`


I'm not sure I would have a test that verifies the "exact same message contained in the command, just copy pasted".

Whenever we want to change a comma for the warning message, the tests will start to fail for no "good" reason.

I would focus on regex matching a couple important part: the readme link, a "high level check" on some important part of the message (eg: /some random error oh no/ or /a valid npx installation/, stuff like this).

The exact wording itself does not seem very important to validate with a UT.

louis-bompart added 5 commits March 29, 2021 19:13

ongoing

b7ae044

merge

97d4ac7

fix bad commit, missing tsconfigs

dcc565a

decorators

f49d7bc

rm async

1dceadb

louis-bompart requested review from olamothe and y-lakhdar as code owners March 30, 2021 03:16

louis-bompart added 3 commits March 29, 2021 23:48

cleaning

c3c8a51

fix tsconfig

3f95cc8

Merge branch 'master' into CDX-195

1e8f0a6

louis-bompart commented Mar 30, 2021

View reviewed changes

packages/cli/package.json

"@types/fs-extra": "^9.0.6",

"@types/jest": "^26.0.22",

Copy link

Collaborator Author

louis-bompart Mar 30, 2021

Choose a reason for hiding this comment

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

To have describe in here.

louis-bompart commented Mar 30, 2021

View reviewed changes

louis-bompart closed this Mar 30, 2021

louis-bompart deleted the CDX-195 branch March 30, 2021 06:02

louis-bompart mentioned this pull request Mar 30, 2021

chore(cli): refactor checks into decorators #122

Merged

y-lakhdar reviewed Mar 30, 2021

View reviewed changes

olamothe reviewed Mar 30, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(cli): refactor checks into decorators #121

chore(cli): refactor checks into decorators #121

louis-bompart commented Mar 30, 2021 •

edited

Loading

github-actions bot commented Mar 30, 2021

louis-bompart Mar 30, 2021

louis-bompart Mar 30, 2021

louis-bompart Mar 30, 2021

louis-bompart Mar 30, 2021

olamothe Mar 30, 2021

louis-bompart Mar 30, 2021

louis-bompart Mar 30, 2021

louis-bompart Mar 30, 2021

louis-bompart Mar 30, 2021

louis-bompart Mar 30, 2021

louis-bompart Mar 30, 2021

louis-bompart Mar 30, 2021 •

edited

Loading

y-lakhdar left a comment

olamothe Mar 30, 2021

olamothe Mar 30, 2021

olamothe Mar 30, 2021

olamothe Mar 30, 2021

olamothe Mar 30, 2021


		static description = `Create a Coveo Headless-powered search page with the React web framework. See ${linkToReadme}`;
		static description =

chore(cli): refactor checks into decorators #121

chore(cli): refactor checks into decorators #121

Conversation

louis-bompart commented Mar 30, 2021 • edited Loading

github-actions bot commented Mar 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louis-bompart Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

y-lakhdar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louis-bompart commented Mar 30, 2021 •

edited

Loading

louis-bompart Mar 30, 2021 •

edited

Loading