update usage of '.sfdx' dir in isvdebugging #4521

RitamAgrawal · 2022-10-28T16:50:25Z

What does this PR do?

This PR adds relativeToolsFolder function in projectPaths which updates the usage of string '.sfdx' and 'tools'

What issues does this PR fix or reference?

@W-11633488@

Functionality Before

.sfdx/tools dir was referenced as string

Functionality After

.sfdx/tools dir is referenced using dynamic functions from salesforcedx-utils-vscode

gbockus-sf · 2022-10-28T19:05:29Z

packages/salesforcedx-vscode-core/src/commands/isvdebugging/bootstrapCmd.ts

    'isvdebuggermdapitmp'
  );
  public readonly relativeApexPackageXmlPath = path.join(
    this.relativeMetdataTempPath,
    'package.xml'
  );
  public readonly relativeInstalledPackagesPath = path.join(
-    '.sfdx',
-    'tools',
+    projectPaths.relativeToolsFolder(),


I realize adding a full blown jest unit test suite for this file is probably more lift then is worthwhile for the work being addressed, but you would add a test(s) to verify the relativeInstalledPackagesPath and relativeMetdataTempPath readonly values are set appropriately.

gbockus-sf

couple of testing suggestions 👍

gbockus-sf · 2022-10-28T19:08:20Z

packages/salesforcedx-utils-vscode/test/jest/helpers/paths.test.ts

+
+    it('should return a path to the relative tools folder', () => {
+      const relativeToolsFolder = projectPaths.relativeToolsFolder();
+      expect(relativeToolsFolder).toEqual(FAKE_TOOLS_FOLDER);


So for this test you should be verifying that the value returned is the combo of the relative state folder and the tools constant. I'd probably be ok with something like

expect(relativeToolsFolder).toEqual( path.join(FAKE_STATE_FOLDER, TOOLS) // TOOLS exported from paths.ts );

But even better would be to mock the projectPaths.relativeStateFolder() response for this test and have something like

relativeStateFolderMock.resolves(FAKE_STATE_FOLDER); const relativeToolsFolder = projectPaths.relativeToolsFolder(); expect(relativeToolsFolder).toEqual( path.join(FAKE_STATE_FOLDER, TOOLS) // TOOLS exported from paths.ts );

Looks like your test is failing with

Expected: ".sfdx/tools" Received: ".sfdx\\tools"```

another good reason to update to the above suggested method for the assertion.

…/github.com/forcedotcom/salesforcedx-vscode into ra/update_usage_of_sfdx_dir_in_isvdebugging

RitamAgrawal · 2022-10-31T17:20:06Z

packages/salesforcedx-utils-vscode/test/jest/helpers/paths.test.ts

@@ -13,6 +13,7 @@ jest.mock('@salesforce/core', () => {
 describe('test project paths', () => {
  const FAKE_WORKSPACE = '/here/is/a/fake/path/to/';
  const FAKE_STATE_FOLDER = '.sfdx';
+  const FAKE_TOOLS_FOLDER = 'tools';


@gbockus-sf Is this okay or Should I import the const TOOLS from projectPaths here?

Importing the tools constant from the single location would def be better

gbockus-sf · 2022-11-02T06:57:23Z

packages/salesforcedx-vscode-core/src/commands/baseDeployRetrieve.ts

@@ -26,7 +26,6 @@ import {
 } from '@salesforce/source-deploy-retrieve/lib/src/client/types';
 import { join } from 'path';
 import * as vscode from 'vscode';
-import { BaseDeployExecutor } from '.';


So doing an import like this results in scanning all things exported from the index.ts file. Which can lead to complications in unit testing. Updated to pull directly from the file where exported instead.

gbockus-sf · 2022-11-02T06:57:55Z

packages/salesforcedx-vscode-core/src/commands/forceSourceDeployManifest.ts

@@ -23,7 +18,7 @@ import { SfdxPackageDirectories } from '../sfdxProject';
 import { telemetryService } from '../telemetry';
 import { workspaceUtils } from '../util';
 import { DeployExecutor } from './baseDeployRetrieve';
-import { FilePathGatherer, SfdxCommandlet, SfdxWorkspaceChecker } from './util';
+import { ConflictDetectionMessages, FilePathGatherer, SfdxCommandlet, SfdxWorkspaceChecker } from './util';


Ran an organize imports. quite a few unused here.

gbockus-sf · 2022-11-02T06:59:21Z

packages/salesforcedx-vscode-core/src/commands/util/conflictDetectionMessages.ts

+ * Licensed under the BSD 3-Clause license.
+ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
+ */
+


So for these two exports they were originally in postconditionCheckers.ts with a ton of other stuff. This lead to always importing this metadata caching checker that had a ton of dependancies and eventually had issues. Simplified by moving these out to their own files and re-exporting from there for index.ts.

gbockus-sf · 2022-11-02T06:59:38Z

packages/salesforcedx-vscode-core/src/commands/util/sfdxCommandlet.ts

 import { channelService } from '../../channels';
 import { notificationService, ProgressNotification } from '../../notifications';
 import { sfdxCoreSettings } from '../../settings';
 import { taskViewService } from '../../statuses';
 import { telemetryService } from '../../telemetry';
 import { workspaceUtils } from '../../util';
+import { EmptyPostChecker } from './emptyPostChecker';


organize imports.

gbockus-sf · 2022-11-02T07:00:03Z

packages/salesforcedx-vscode-core/test/jest/commands/isvdebugging/bootstrapCmd.test.ts

  });

  it('should be defined', () => {
+    isvDebugBootstrapExecutorInst = new IsvDebugBootstrapExecutor();


So this wasn't required, but when testing an instance of a class it's always good to create the instance for each test.

gbockus-sf · 2022-11-02T07:03:54Z

scripts/setup-jest.ts

+        onDidChange: jest.fn(),
+        onDidCreate: jest.fn(),
+        onDidDelete: jest.fn()
+      }),


new vscode mock setup to get things working for core. Note that b/c the src/context/index.ts creates an instance of WorkspaceContext on initialization
export const workspaceContext = WorkspaceContext.getInstance();
We need to mock the file watcher results prior to loading any test dependancies.
This is an anti-pattern we should avoid for singletons.

gbockus-sf · 2022-11-02T07:04:16Z

packages/salesforcedx-vscode-core/src/commands/baseDeployRetrieve.ts

@@ -26,7 +26,6 @@ import {
 } from '@salesforce/source-deploy-retrieve/lib/src/client/types';


Some touches to get jest unit tests working in core.

gbockus-sf · 2022-11-04T05:26:21Z

packages/salesforcedx-vscode-core/src/commands/isvdebugging/bootstrapCmd.ts

@@ -53,19 +54,21 @@ export interface InstalledPackageInfo {
  versionNumber: string;
 }

+export const ISVDEBUGGER = 'isvdebuggermdapitmp';
+export const INSTALLED_PACKAGES = 'installed-packages';
+export const PACKAGE_XML = 'package.xml';


we should update the below paths to use these constants.

gbockus-sf · 2022-11-04T05:41:28Z

packages/salesforcedx-utils-vscode/src/helpers/paths.ts

-  relativeStateFolder
+  relativeStateFolder,
+  relativeToolsFolder,
+  TOOLS


nit: I'd just export the TOOLS constant on line 17 vs including constants and functions here.

gbockus-sf

Couple of simple polish suggestions and you're good to go 👍

Verified unit test execution and pass in circle ✅
Verified unit test execution local ✅
Not sure how to execute the ISV debugger but verified as best I could ✅

chore: add relativeToolsFolder

d8cb4ff

RitamAgrawal requested a review from a team as a code owner October 28, 2022 16:50

RitamAgrawal requested a review from jeffb-sfdc October 28, 2022 16:50

Merge branch 'develop' into ra/update_usage_of_sfdx_dir_in_isvdebugging

026b65c

gbockus-sf reviewed Oct 28, 2022

View reviewed changes

RitamAgrawal and others added 4 commits October 29, 2022 04:33

refactor: updated paths.test.ts

6d0d7f8

Merge branch 'ra/update_usage_of_sfdx_dir_in_isvdebugging' of https:/…

7c9418a

…/github.com/forcedotcom/salesforcedx-vscode into ra/update_usage_of_sfdx_dir_in_isvdebugging

Merge branch 'develop' into ra/update_usage_of_sfdx_dir_in_isvdebugging

e31b2fa

Merge branch 'develop' into ra/update_usage_of_sfdx_dir_in_isvdebugging

dcd8635

RitamAgrawal commented Oct 31, 2022

View reviewed changes

RitamAgrawal and others added 4 commits November 1, 2022 00:09

refactor: paths.test file

ca25580

test: add jest test to core package

10a623c

fix: lint error

f267555

test: updates for running jest unit test in core

fb31e80

gbockus-sf reviewed Nov 2, 2022

View reviewed changes

RitamAgrawal and others added 3 commits November 2, 2022 23:21

test: add more jest test

d41019e

Merge branch 'develop' into ra/update_usage_of_sfdx_dir_in_isvdebugging

4d24cf1

Merge branch 'develop' into ra/update_usage_of_sfdx_dir_in_isvdebugging

10721ac

gbockus-sf reviewed Nov 4, 2022

View reviewed changes

gbockus-sf approved these changes Nov 4, 2022

View reviewed changes

RitamAgrawal merged commit d575ba8 into develop Nov 4, 2022

RitamAgrawal deleted the ra/update_usage_of_sfdx_dir_in_isvdebugging branch November 4, 2022 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update usage of '.sfdx' dir in isvdebugging #4521

update usage of '.sfdx' dir in isvdebugging #4521

RitamAgrawal commented Oct 28, 2022

gbockus-sf Oct 28, 2022

gbockus-sf left a comment

gbockus-sf Oct 28, 2022

gbockus-sf Oct 28, 2022

RitamAgrawal Oct 31, 2022

gbockus-sf Oct 31, 2022

gbockus-sf Nov 2, 2022

gbockus-sf Nov 2, 2022

gbockus-sf Nov 2, 2022

gbockus-sf Nov 2, 2022

gbockus-sf Nov 2, 2022

gbockus-sf Nov 2, 2022

gbockus-sf Nov 2, 2022

gbockus-sf Nov 4, 2022

gbockus-sf Nov 4, 2022

gbockus-sf left a comment

		@@ -26,7 +26,6 @@ import {
		} from '@salesforce/source-deploy-retrieve/lib/src/client/types';

update usage of '.sfdx' dir in isvdebugging #4521

update usage of '.sfdx' dir in isvdebugging #4521

Conversation

RitamAgrawal commented Oct 28, 2022

What does this PR do?

What issues does this PR fix or reference?

Functionality Before

Functionality After

Choose a reason for hiding this comment

gbockus-sf 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

gbockus-sf left a comment

Choose a reason for hiding this comment