Chore: Add jest unit test for salesforcedx-utils package #4470

gbockus-sf · 2022-10-01T06:09:39Z

What does this PR do?

Create the first set of jest unit tests that are using purely jest for unit testing.

What issues does this PR fix or reference?

@W-11309496@

Functionality Before

No unit tests for salesforcedx-utils.

Functionality After

Unit test coverage for utils package.
Example of how to write tests using jest expects instead of mocha/chai

gbockus-sf · 2022-10-01T06:10:51Z

config/jest.base.config.js

@@ -2,7 +2,7 @@
 module.exports = {
  preset: 'ts-jest',
  testEnvironment: 'node',
-  testMatch: [ "**/unit/**/?(*.)+(spec|test).[t]s?(x)" ],
+  testMatch: [ "**/(unit|jest)/**/?(*.)+(spec|test).[t]s?(x)" ],


unit tests will now be valid under a unit or jest directory.
The intention is the jest directory is the new location for jest true unit tests.

gbockus-sf · 2022-10-01T06:11:07Z

packages/salesforcedx-utils/jest.config.js

-  {
-    testMatch: [ "**/unit/**/?(*.)+(spec|test).[t]s?(x)" ],
-  }
+  baseConfig


clean up unnecessary override.

👍🏽
Also, TS is complaining about top line of this file for me:

cool..yeah it's a js file so can be ignored. I can add a ignore comment I think

just an FYI this was only showing u in the utils projects which using eslint vs tslint. So I only updated that config file

gbockus-sf · 2022-10-01T06:11:29Z

packages/salesforcedx-utils/src/cli/cliCommandExecution.ts

+export const NO_STDOUT_ERROR = 'No stdout found for childProcess';
+export const NO_STDERR_ERROR = 'No stderr found for childProcess';
+export const CANCELATION_INTERVAL = 1000;
+export const KILL_CODE = 'SIGKILL';


exported constants so they can be verified in unit tests.

gbockus-sf · 2022-10-01T06:12:04Z

packages/salesforcedx-utils/src/cli/cliCommandExecutor.ts

@@ -83,79 +74,3 @@ export class CliCommandExecutor {
    );
  }
 }
-
-export class CompositeCliCommandExecutor {


put class in it's own file for cleaner organization and import.

gbockus-sf · 2022-10-01T06:12:34Z

packages/salesforcedx-utils/src/cli/compositeCliCommandExecution.ts

+import { CommandExecution } from '../types/commandExecution';
+import { Command } from './command';
+
+export class CompositeCliCommandExecution implements CommandExecution {


nothing new here just moved to a separate file.

gbockus-sf · 2022-10-01T06:12:58Z

packages/salesforcedx-utils/src/cli/compositeCliCommandExecutor.ts

+import { Command } from './command';
+import { CompositeCliCommandExecution } from './compositeCliCommandExecution';
+
+export class CompositeCliCommandExecutor {


same story: 1 class exported to its own file.

gbockus-sf · 2022-10-01T06:14:11Z

packages/salesforcedx-utils/src/cli/forceConfigGet.ts

@@ -10,7 +10,8 @@ import { CommandOutput } from './commandOutput';
 import { SfdxCommandBuilder } from './sfdxCommandBuilder';
 /**
 * @deprecated
- * NOTE: This code is deprecated in favor of using ConfigUtil.ts


I'm not sure why I deleted this NOTE honestly. Probably need to visit if this command is necessary.

Might be good to put this back? Or add to the same line as deprecated

ok..yeah I'll put it back. This is only used in the debuggers currently that can't access configUtils in the vscode-utils package FYI.

gbockus-sf · 2022-10-01T06:14:46Z

packages/salesforcedx-utils/src/cli/sfdxCommandBuilder.ts

@@ -1,7 +1,8 @@
 import { CommandBuilder } from './commandBuilder';

+export const SFDX_COMMAND = 'sfdx';


I should probably put this in constants.

Yeah, good idea!

gbockus-sf · 2022-10-01T06:15:20Z

packages/salesforcedx-utils/src/i18n/localization.ts

@@ -27,59 +19,3 @@ export class Localization implements LocalizationProvider {
    return this.delegate.localize(label, ...args);
  }
 }
-
-export type MessageBundle = {


same story as above. Organize by putting exports in their own files.

gbockus-sf · 2022-10-01T06:18:08Z

packages/salesforcedx-utils/test/jest/cli/__snapshots__/command.test.ts.snap

@@ -0,0 +1,5 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+


our first snapshot. These are generated by jest the first time a shouldMatchSnapshot type call is made. Then after the value found here is compare on the matchSnapshot call in future tests. If it doesn't match the assert will fail. If the value has change you can updated it with a flag passed to the test (included in the failure output).

Thing to take away is snapshot should be treated as code and reviewed closely on change

gbockus-sf · 2022-10-01T06:19:09Z

packages/salesforcedx-utils/test/jest/cli/cliCommandExecution.test.ts

+} from '../../../src/cli/cliCommandExecution';
+import { Observable } from 'rxjs/Observable';
+import * as kill from 'tree-kill';
+jest.mock('tree-kill');


This mocks the module in memory in jest. So when it's used in the code under test the underlying functions are just mocks.

So this will become a standard practice for us to use across jest tests?

correct. If you need to mock and entire module for a test this is the easiest way to do it.

gbockus-sf · 2022-10-01T06:19:33Z

packages/salesforcedx-utils/test/jest/cli/cliCommandExecution.test.ts

+import * as kill from 'tree-kill';
+jest.mock('tree-kill');
+
+const treeKillMocked = jest.mocked(kill);


This is how you correctly type the mocked function for use in the test while keeping typescript happy.

gbockus-sf · 2022-10-01T06:20:06Z

packages/salesforcedx-utils/test/jest/cli/cliCommandExecution.test.ts

+    } as any);
+    intervalSpy = jest.spyOn(Observable, 'interval').mockReturnValue({
+      subscribe: subscribeSpy
+    } as any);


This is testing a class that uses rxjs. It leads to some pretty strange tests.

gbockus-sf · 2022-10-01T06:20:42Z

packages/salesforcedx-utils/test/jest/cli/cliCommandExecution.test.ts

+  });
+
+  afterEach(() => {
+    jest.resetAllMocks();


I've worked on projects previously where we set this in the global after for all tests. I'm not sure if that's the right move or not but we could put that in place.

That would be cool. Is there ever a situation where one would want to not have this happen after a test?

No I wouldn't think so. It wouldn't be goodto have one test depending on setup from another so I think we could just reset all Mocks by default.

Yep sounds good to me!

gbockus-sf · 2022-10-01T06:21:50Z

packages/salesforcedx-utils/test/jest/cli/cliCommandExecution.test.ts

+      testCommand,
+      testChildProcess
+    );
+    expect(cliCommandExecution).toBeInstanceOf(CliCommandExecution);


note that when using jest it automatically injects jest and it's components like expect.

OK so that means they don't have to be explicitly imported?

gbockus-sf · 2022-10-01T06:22:50Z

packages/salesforcedx-utils/test/jest/cli/cliCommandExecution.test.ts

+      'data'
+    ]);
+
+    expect(subscribeSpy).toHaveBeenCalledTimes(2);


There are a ton of good options for jest expects: https://jestjs.io/docs/expect

gbockus-sf · 2022-10-01T06:23:37Z

packages/salesforcedx-utils/test/jest/cli/cliCommandExecution.test.ts

+          testChildProcess,
+          testCancelationToken
+        );
+      }).toThrowError(NO_PID_ERROR);


This is the syntax for verifying that a function call throws an error. It's a little weird that you have to wrap the call in an arrow function (I almost always forget).

This would be good to add in tests.md

Better yet, add an instruction to create a snippet... or maybe we could ship some 🤔

👍 I'll follow on this PR with an update to tests.md

gbockus-sf · 2022-10-01T06:24:39Z

packages/salesforcedx-utils/test/jest/cli/cliCommandExecution.test.ts

+      expect(subscribeSpy).toHaveBeenCalledTimes(3);
+      const timoutHandler = subscribeSpy.mock.calls[2][0];
+      const timeoutPromise = timoutHandler();
+      expect(treeKillMocked).toHaveBeenCalledTimes(1);


note how we can use the treeKillMocked here to verify that an dependent module was called.

gbockus-sf · 2022-10-01T06:25:20Z

packages/salesforcedx-utils/test/jest/cli/cliCommandExecution.test.ts

+        killCallback();
+      } else {
+        fail('Should have had a kill callback function.');
+      }


The treekill function is odd in that it takes a callback as a parameter. To test the callback code I had to call it here.

gbockus-sf · 2022-10-01T06:25:57Z

packages/salesforcedx-utils/test/jest/cli/cliCommandExecutor.test.ts

+import { CliCommandExecution } from '../../../src/cli/cliCommandExecution';
+import { GlobalCliEnvironment } from '../../../src/cli/globalCliEnvironment';
+jest.mock('cross-spawn');
+jest.mock('../../../src/cli/cliCommandExecution');


Note this is a mock for a third party module vs a local module.

gbockus-sf · 2022-10-01T06:26:45Z

packages/salesforcedx-utils/test/jest/cli/cliCommandExecutor.test.ts

+
+  beforeEach(() => {
+    // Add a global value to the GCE option for processing during creation.
+    GlobalCliEnvironment.environmentVariables.set(globalKey, globalValue);


this is an exported Map so I decided to just set a value verse mocking the Map functionality.

gbockus-sf · 2022-10-01T06:27:21Z

packages/salesforcedx-utils/test/jest/cli/cliCommandExecutor.test.ts

+      fakeCommand,
+      fakeChildProcess,
+      undefined
+    );


note we're not calling the underlying executor here but just verifying that it should have been called.

gbockus-sf · 2022-10-03T15:39:27Z

packages/salesforcedx-utils/test/jest/cli/forceConfigGet.test.ts

+  let getCmdResultSpy: jest.SpyInstance;
+  beforeEach(() => {
+    executeSpy = jest
+      .spyOn(CliCommandExecutor.prototype, 'execute')


jest.spyOn =~ sandbox.stub

gbockus-sf · 2022-10-03T15:40:44Z

packages/salesforcedx-utils/test/jest/cli/forceConfigGet.test.ts

+      .mockReturnValue(fakeExecution as any);
+    getCmdResultSpy = jest
+      .spyOn(CommandOutput.prototype, 'getCmdResult')
+      .mockResolvedValue(fakeResult);


Note: mockResolved value returns a promise wrapped results. mockReturnValue is without promises.

gbockus-sf · 2022-10-03T15:42:05Z

packages/salesforcedx-utils/test/jest/cli/forceConfigGet.test.ts

+      getCmdResultSpy.mockResolvedValue(partialJson);
+      const forceConfigGet = new ForceConfigGet();
+      // Unexpected token error is thrown b/c json can not be parsed.
+      expect(forceConfigGet.getConfig(fakePath)).rejects.toThrowError(/Unexpected token/);


This is the syntax for checking if an error is thrown by making a call.

gbockus-sf · 2022-10-03T16:05:49Z

packages/salesforcedx-utils/test/jest/cli/commandBuilder.test.ts

+    expect(commandBuilderInst).toBeInstanceOf(CommandBuilder);
+    const testCommand = commandBuilderInst.build();
+    expect(testCommand).toBeInstanceOf(Command);
+    expect(testCommand.command).toBe(testCommandStr);


Note: I sort of figured out as I went that .toBe and .toEqual are pretty much the same but toBe won't work sometimes which led me to search for the diff where I found https://dev.to/thejaredwilcurt/why-you-should-never-use-tobe-in-jest-48ca#:~:text=toEqual%20and%20the%20.,equality%20fails%20on%20non%2Dprimatives.

So bottom line we should probably just default to toEqual
I'll update these tests to get rid of toBe

gbockus-sf · 2022-10-03T16:09:01Z

packages/salesforcedx-utils/test/jest/requestService/__snapshots__/requestService.test.ts.snap

+    },
+  ],
+}
+`;


much more interesting snapshots here. Note here it is useful b/c I don't have to check all the properties individually or in a constructed object in the call like headers, data, etc.

gbockus-sf · 2022-10-03T16:09:12Z

packages/salesforcedx-utils/test/jest/requestService/__snapshots__/requestService.test.ts.snap

+        },
+        "timeout": 20000,
+        "type": "POST",
+        "url": "https://where.is/the/instance/https://this.is.a.test",


I'll verify why this looks weird.

gbockus-sf · 2022-10-03T16:10:35Z

packages/salesforcedx-utils/test/jest/requestService/requestService.test.ts

+      const result = await requestServiceInst.execute(testCommand);
+      expect(result).toBe(fakeResponse.responseText);
+      expect(sendRequestMock).toHaveBeenCalled();
+      expect(sendRequestMock).toMatchSnapshot();


I should probably update this to snapshot on the args and not the entire mock

gbockus-sf · 2022-10-05T20:48:50Z

packages/salesforcedx-utils/test/jest/cli/command.test.ts

+      .withLogName('testLogName');
+
+    const command = new Command(commandBuilderInst);
+    expect(command.toString()).toMatchSnapshot();


example snapshot expect

randi274

Mostly questions and spelling; thanks for getting all of this ramped up for us!

randi274 · 2022-10-06T16:51:06Z

packages/salesforcedx-utils/src/cli/commandBuilder.ts

@@ -7,6 +7,10 @@

 import { Command } from './command';

+export const JSON_FLAG = '--json';


Love this centralized place for these. Our libraries could do with the same treatment, if we were investing there.

randi274 · 2022-10-06T16:52:04Z

packages/salesforcedx-utils/src/cli/forceConfigGet.ts

@@ -10,7 +10,8 @@ import { CommandOutput } from './commandOutput';
 import { SfdxCommandBuilder } from './sfdxCommandBuilder';
 /**
 * @deprecated
- * NOTE: This code is deprecated in favor of using ConfigUtil.ts


Might be good to put this back? Or add to the same line as deprecated

randi274 · 2022-10-06T16:53:13Z

packages/salesforcedx-utils/src/cli/sfdxCommandBuilder.ts

@@ -1,7 +1,8 @@
 import { CommandBuilder } from './commandBuilder';

+export const SFDX_COMMAND = 'sfdx';


Yeah, good idea!

randi274 · 2022-10-06T16:54:37Z

packages/salesforcedx-utils/src/i18n/message.ts

+    if (args.length >= 1) {
+      const expectedNumArgs = possibleLabel.split('%s').length - 1;
+      if (args.length !== expectedNumArgs) {
+        // just log it, we might want to hide some in some languges on purpose


nit: typo on languages

randi274 · 2022-10-06T16:54:57Z

packages/salesforcedx-utils/src/i18n/message.ts

+        }
+      }
+
+      return format(possibleLabel, ...labelArgs); // TODO: verify this works


..does it? :)

randi274 · 2022-10-06T17:05:03Z

packages/salesforcedx-utils/test/jest/cli/command.test.ts

+    expect(command.logName).toBeUndefined();
+  });
+
+  it('Should be abled to pass a fully built builder.', () => {


nit: typo on abled

randi274 · 2022-10-06T17:05:39Z

packages/salesforcedx-utils/test/jest/cli/command.test.ts

+    expect(command.toString()).toEqual(testDescription);
+  });
+
+  it('Should construct string if no descirption provided.', () => {


nit: typo on description

In this case, are we building a command too? Test name is a little unclear

yeah this is verifying that the toString method works as expected.

randi274 · 2022-10-06T17:08:11Z

packages/salesforcedx-utils/test/jest/cli/commandOutput.test.ts

+  let commandOutput: CommandOutput;
+  let result: Promise<string>;
+
+  // It's a pain to unit test Obserables.  Open to exploring other options.


nit: Obserables -> Observables

randi274 · 2022-10-06T17:09:21Z

packages/salesforcedx-utils/test/jest/cli/commandOutput.test.ts

@@ -0,0 +1,102 @@
+import { CommandOutput } from '../../../src';
+
+describe('CommandOuput Unit Tests.', () => {


nit: CommandOuput -> CommandOutput

randi274 · 2022-10-06T18:49:18Z

packages/salesforcedx-utils/test/jest/i18n/message.test.ts

+      expect(label).toEqual(expected);
+    });
+
+    it('Should only partially populate if too few args are passed.', () => {


shouldn't it also log an error, or was that elsewhere?

I didn't make the rules...just tested that it works they way it was coded :)

klewis-sfdc · 2022-10-12T22:14:29Z

packages/salesforcedx-utils/src/requestService/requestService.ts

@@ -32,12 +34,12 @@ export class RequestService {
  private _proxyAuthorization!: string;
  private _connectionTimeoutMs: number = DEFAULT_CONNECTION_TIMEOUT_MS;

-  public getEnvVars(): any {
+  public getEnvVars(): NodeJS.ProcessEnv {


klewis-sfdc · 2022-10-12T23:01:45Z

packages/salesforcedx-utils/test/jest/cli/commandBuilder.test.ts

+  it('Should have special handling for the json flag.', () => {
+    const testCommand = commandBuilderInst.withArg(JSON_FLAG);
+    expect(testCommand.args).toEqual(
+      expect.arrayContaining([JSON_FLAG, LOG_LEVEL_FLAG, FATAL])


Coool: https://jestjs.io/docs/expect#expectarraycontainingarray

klewis-sfdc · 2022-10-13T00:11:10Z

packages/salesforcedx-utils/test/jest/i18n/message.test.ts

+      const labelKey = 'notAKey';
+      const label = message.localize(labelKey);
+      const expected = `${MISSING_LABEL_MSG} ${labelKey}`;
+      expect(label).toEqual(expected);


It seems like these tests execute a console.warn that ends up being reported when npm run test:unit is run. Is that correct? Could something be stubbed so that this console.warn does not get output when the tests are run?

that is correct...I'll see what I can do

klewis-sfdc

Looks great to me, Gordon. Just the one question about the console.warn thing. Thanks for bringing jest into the fold!

gbockus-sf added 2 commits October 1, 2022 00:54

chore: add first set of jest tests for utils

7de669e

chore: add remaining unit tests

8d8b1fd

gbockus-sf requested a review from a team as a code owner October 1, 2022 06:09

gbockus-sf requested a review from CristiCanizales October 1, 2022 06:09

gbockus-sf commented Oct 1, 2022

View reviewed changes

gbockus-sf commented Oct 3, 2022

View reviewed changes

gbockus-sf and others added 4 commits October 3, 2022 11:17

chore: update snapshot to use args instead of mock. fix url in test

33dd810

chore: change toBe to toEqual in jest tests

9d9826d

Merge branch 'develop' into gbockus/jest-tests-for-utils-1

a52f696

Merge branch 'develop' into gbockus/jest-tests-for-utils-1

dea16dc

gbockus-sf commented Oct 5, 2022

View reviewed changes

randi274 reviewed Oct 6, 2022

View reviewed changes

gbockus-sf and others added 4 commits October 7, 2022 16:52

Merge branch 'develop' into gbockus/jest-tests-for-utils-1

6325cf2

chore: updates from review

4517270

chore: more followup from review

12305b9

Merge branch 'develop' into gbockus/jest-tests-for-utils-1

06f3d5c

klewis-sfdc reviewed Oct 12, 2022

View reviewed changes

klewis-sfdc reviewed Oct 13, 2022

View reviewed changes

klewis-sfdc approved these changes Oct 13, 2022

View reviewed changes

chore: changes from review

5bedcf4

gbockus-sf merged commit b12b2f7 into develop Oct 14, 2022

gbockus-sf deleted the gbockus/jest-tests-for-utils-1 branch October 14, 2022 15:05

		@@ -1,7 +1,8 @@
		import { CommandBuilder } from './commandBuilder';

		export const SFDX_COMMAND = 'sfdx';

		@@ -7,6 +7,10 @@

		import { Command } from './command';

		export const JSON_FLAG = '--json';

		@@ -0,0 +1,102 @@
		import { CommandOutput } from '../../../src';

		describe('CommandOuput Unit Tests.', () => {

Chore: Add jest unit test for salesforcedx-utils package #4470

Chore: Add jest unit test for salesforcedx-utils package #4470

Conversation

gbockus-sf commented Oct 1, 2022 • edited

What does this PR do?

What issues does this PR fix or reference?

Functionality Before

Functionality After

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randi274 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

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

klewis-sfdc Oct 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klewis-sfdc left a comment

Choose a reason for hiding this comment

gbockus-sf commented Oct 1, 2022 •

edited

klewis-sfdc Oct 13, 2022 •

edited