refactor: update apex debugger sfdx hard coded reference #4509
Conversation
| expect(projectPaths.sfdxProjectConfig).toBeDefined(); | ||
| }); | ||
|
|
||
| // todo: this is failing as it appears stateFolder is not being replaced. |
There was a problem hiding this comment.
@gbockus-sf this was being super silly, and it looked like the stateFolder() function wasn't being mocked. Any thoughts on why that simple setup might be failing?
There was a problem hiding this comment.
Tricky tricky. So in your test you're mocking projectPaths.stateFolder, but in the code under test it's calling the stateFolder function by name verse via the projectPaths object. You just need to update the code under test and you'll be in business
const pathToSFDXProjectConfig = path.join(
projectPaths.stateFolder(),
SFDX_CONFIG
);
return pathToSFDXProjectConfig;
}
There was a problem hiding this comment.
Ohhh that is tricky! Let me push what I think this should be and let me know if it looks right
| @@ -25,6 +21,7 @@ const DEBUG = 'debug'; | |||
| const LOGS = 'logs'; | |||
| const APEX_DB = 'apex.db'; | |||
| const LWC = 'lwc'; | |||
| const SFDX_CONFIG = 'sfdx-config.json'; | |||
There was a problem hiding this comment.
nit: can we call this SFDX_CONFIG_FILE? Kind of a shame we can't get that from core somewhere.
| SFDX_STATE_FOLDER: '.sfdx' | ||
| } | ||
| }; | ||
| }); |
There was a problem hiding this comment.
this was a bit of an adventure to figure out lol
| }); | ||
|
|
||
| it('returns path to the state folder if the project does not have a root workspace', () => { | ||
| hasRootWorkspaceStub = jest.spyOn(workspaceUtils, 'hasRootWorkspace').mockReturnValue(false); |
There was a problem hiding this comment.
I'm move the hasRootWorkspaceStub = jest.spyOn(workspaceUtils, 'hasRootWorkspace') to the beforeEach since you use it in multiple tests and just have
hasRootWorkspaceStub.mockReturnValue(false) in each test. Makes for easier reading
| getRootWorkspacePathStub = jest.spyOn(workspaceUtils, 'getRootWorkspacePath').mockReturnValue(FAKE_WORKSPACE); | ||
| }); | ||
|
|
||
| it('is defined', () => { |
There was a problem hiding this comment.
nit: curious on your thoughts. We've have somewhat of a standard of having all unit tests follow the should do a thing pattern where each it starts with should. Do you think we follow that pattern here?
There was a problem hiding this comment.
oh! Yes I can change that. This pattern felt more intuitive/declarative, but I'm not opposed to the extra chars of the "should"
| "test:vscode-insiders-integration": "cross-env CODE_VERSION=insiders npm run test:vscode-integration" | ||
| "test:vscode-insiders-integration": "cross-env CODE_VERSION=insiders npm run test:vscode-integration", | ||
| "test:unit": "jest --coverage", | ||
| "coverage": "jest --coverage" |
There was a problem hiding this comment.
I don't think this coverage script is necessary. It was in one package b/c of how we did coverage previously, but probably doesn't need to be duplicated across packages.
There was a problem hiding this comment.
This is also present in utils and utils-vscode, but I removed from this package.
| import * as vscode from 'vscode'; | ||
| import { DebugProtocol } from 'vscode-debugprotocol'; | ||
| import { DebugConfigurationProvider } from './adapter/debugConfigurationProvider'; | ||
| import { setupGlobalDefaultUserIsvAuth } from './context'; | ||
| import { registerIsvAuthWatcher, setupGlobalDefaultUserIsvAuth } from './context'; |
There was a problem hiding this comment.
beautiful refactor...lots of red for 1 green
| expect(onDidCreateSpy).toHaveBeenCalledWith(expect.any(Function)); | ||
| expect(onDidDeleteSpy).toHaveBeenCalledWith(expect.any(Function)); | ||
| expect(pushSpy).toHaveBeenCalled(); | ||
| }); |
| function toolsFolder(): string { | ||
| const pathToToolsFolder = path.join(stateFolder(), TOOLS); | ||
| const pathToToolsFolder = path.join(projectPaths.stateFolder(), TOOLS); |
There was a problem hiding this comment.
@gbockus-sf here's that change, which did fix the test - I updated the other spots using stateFolder too so the path to test those is easier
|
clicked update branch with latest to pick up the version change |
There was a problem hiding this comment.
Looks great.
QE complete
- Verify that new jest unit tests run locally in salesforcedx-vscode-apex-debugger package ✅
- Verify that watcher gets config file from uitls ✅
- Verified configFile value via break point in value: '/Users/gbockus/github/PDT/SFProjects/ebikes-lwc-2/ebikes-lwc/.sfdx/sfdx-config.json' ✅
- Verified watcher fires when config file changes ✅
- Verified vscode-apex-debugger unit tests in circle ✅
- salesforcedx-vscode-apex-debugger: registerIsvAuthWatcher
salesforcedx-vscode-apex-debugger: √ should be defined (2 ms)
salesforcedx-vscode-apex-debugger: √ should not watch files if workspace folders are not present (1 ms)
salesforcedx-vscode-apex-debugger: √ should watch files if workspace folders are present (3 ms)
salesforcedx-vscode-apex-debugger: ---------------|---------|----------|---------|---------|-------------------
salesforcedx-vscode-apex-debugger: File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
salesforcedx-vscode-apex-debugger: ---------------|---------|----------|---------|---------|-------------------
salesforcedx-vscode-apex-debugger: All files | 60 | 33.33 | 28.57 | 65 |
salesforcedx-vscode-apex-debugger: index.ts | 100 | 100 | 50 | 100 |
salesforcedx-vscode-apex-debugger: isvContext.ts | 54.54 | 33.33 | 20 | 63.15 | 12-35
salesforcedx-vscode-apex-debugger: ---------------|---------|----------|---------|---------|-------------------
- salesforcedx-vscode-apex-debugger: registerIsvAuthWatcher
What does this PR do?
Updates the apex debugger package to remove a hard-coded CLI configuration value.
What issues does this PR fix or reference?
@W-11633542@
Functionality Before
We were using a hard-coded reference to the path for the sfdx-config.json.
Functionality After
Remove hard-coded
sfdxpaths from the apex debugger package, and adds complimentary jest tests!