fix: soql builder should not error when no default org #4847
Conversation
| run(true); | ||
| run(false); |
There was a problem hiding this comment.
Stole it from someone on SO :)
| @@ -1,3 +1,9 @@ | |||
| /* | |||
| * Copyright (c) 2020, salesforce.com, inc. | |||
There was a problem hiding this comment.
One of the auto-formatters/linters added this copyright. I can try to change it, but it might complain.
| describe('withSFConnection', () => { | ||
| function run(showErrorMessage: boolean) { | ||
| it(`should ${ | ||
| showErrorMessage ? '' : 'not ' |
There was a problem hiding this comment.
Interesting formatting - was this intentional?
There was a problem hiding this comment.
This was Prettier's doing. I agree, it is "interesting." Let me see if this is enforced or was just a suggestion.
There was a problem hiding this comment.
So turns out that the lint script only cares about src and this is test, so technically I could revert it and CI would pass, but it underlines red locally. It's ugly, but I'll leave it since it's following the linting rules.
|
This PR makes me so, so happy. Would it be possible to add a Jest test to the bit of code that was added? |
| @@ -0,0 +1,54 @@ | |||
| /* | |||
There was a problem hiding this comment.
@randi274 I added unit tests here. I spent way too long trying to make the onOrgChangeDefaultHandler test a true unit test and mock withSFConnection (its direct dependant) instead of debouncedShowChannelAndErrorMessage(two levels down), but Jest was just not liking it. So, now we have two unit tests that are largely similar. I'm fine keeping them both, killing one, or combing.
There was a problem hiding this comment.
Hmm... and CI just ran and failed on this:
TypeError: Cannot read properties of undefined (reading 'appendLine')
Looks like that is coming from an async timer calling that code outside of the test's mocks. That did not happen locally, but assuming it's a race condition because the timer had longer to run on CI.
I'll dig more into this tomorrow.
There was a problem hiding this comment.
FYI I was able to repo locally using the following:
ansible|~/github/PDT/salesforcedx-vscode (rb/soql-default-org-no-error ✔) node 'node_modules/.bin/jest' '/Users/gbockus/github/PDT/salesforcedx-vscode/packages/salesforcedx-vscode-soql/test/jest/sfdx.test.ts' -c '/Users/gbockus/github/PDT/salesforcedx-vscode/packages/salesforcedx-vscode-soql/jest.config.js' -t 'onOrgChangeDefaultHandler'
PASS packages/salesforcedx-vscode-soql/test/jest/sfdx.test.ts (5.203 s)
withSFConnection
○ skipped should show error message when showErrorMessage=true
○ skipped should not show error message when showErrorMessage=false
onOrgChangeDefaultHandler
✓ should show connection error message when default org is set (2 ms)
✓ should not show connection error message when default org is not set (1 ms)
Test Suites: 1 passed, 1 total
Tests: 2 skipped, 2 passed, 4 total
Snapshots: 0 total
Time: 5.318 s, estimated 6 s
Ran all test suites matching /\/Users\/gbockus\/github\/PDT\/salesforcedx-vscode\/packages\/salesforcedx-vscode-soql\/test\/jest\/sfdx.test.ts/i with tests matching "onOrgChangeDefaultHandler".
/Users/gbockus/github/PDT/salesforcedx-vscode/packages/salesforcedx-utils-vscode/out/src/commands/channelService.js:84
this.channel.appendLine(text);
^
TypeError: Cannot read properties of undefined (reading 'appendLine')
at ChannelService.appendLine (/Users/gbockus/github/PDT/salesforcedx-vscode/packages/salesforcedx-utils-vscode/src/commands/channelService.ts:106:18)
at showChannelAndErrorMessage (/Users/gbockus/github/PDT/salesforcedx-vscode/packages/salesforcedx-vscode-soql/src/sfdx.ts:18:28)
at Timeout.later [as _onTimeout] (/Users/gbockus/github/PDT/salesforcedx-vscode/packages/salesforcedx-vscode-soql/node_modules/debounce/index.js:27:23)
at listOnTimeout (node:internal/timers:559:17)
at processTimers (node:internal/timers:502:7)
ansible|~/github/PDT/salesforcedx-vscode (rb/soql-default-org-no-error ✔) echo $?
1
ansible|~/github/PDT/salesforcedx-vscode (rb/soql-default-org-no-error ✔)
There was a problem hiding this comment.
Awesome, thanks @gbockus-sf ! That worked for me now. Interesting that it doesn't happen with the jest CLI does does calling it from node.
There was a problem hiding this comment.
Fixed it up. The issue was that I was not setting .mockImplementation(() => Promise.resolve()) so the real impl was getting called async.
| @@ -68,12 +72,15 @@ export async function retrieveSObject( | |||
| return name; | |||
| } | |||
|
|
|||
| workspaceContext.onOrgChange(async (orgInfo: any) => { | |||
| export async function onOrgChangeDefaultHandler(orgInfo: any) { | |||
There was a problem hiding this comment.
Extracted this out for testability.
|
I QA'd this and this should be good to merge in. |
What does this PR do?
Suppresses the
error_connection("We can’t query your org") toast message when no default org is set when opening VS Code. It retains the message in other cases, such as when actually opening the SOQL builder or trying to query the org.What issues does this PR fix or reference?
@W-13131914@
Functionality Before
When opening VS Code without a default org set, the SOQL extension would display an error on project load. Additional errors also shown when trying to open the SOQL builder or run a SOQL query.
Functionality After
When opening VS Code without a default org set, no error is shown on project load. Errors are only shown when trying to open the SOQL builder or run a SOQL query.