fix: push-on-save working as expected, diagnostics cleared after successful push #4975
Conversation
| @@ -128,6 +128,7 @@ export class DeployQueue { | |||
| default: | |||
| displayError(e.message); | |||
| } | |||
| } finally { | |||
There was a problem hiding this comment.
DeployQueue.locked is set to false. What does this mean?
There was a problem hiding this comment.
this unlocks the queue in order to let the subsequent pushes on save be triggered
| mockDeployResult | ||
| ); | ||
| expect(deployRetrieveExecutorClearSpy).toHaveBeenCalled(); | ||
| expect(sfdxCommandletExecutorClearSpy).toHaveBeenCalled(); |
There was a problem hiding this comment.
Is the sequence in which the error collection is cleared important?
There was a problem hiding this comment.
no, but I did it different in the 2 files I implemented this according to the file... e.g: in baseDeployRetrieve I clear first the deploy errors, and in forceSourcePush I clear first the push errors
| const executor = new TestDeployExecutor( | ||
| 'testDeploy', | ||
| 'force_source_deploy_with_sourcepath_beta' | ||
| ); |
There was a problem hiding this comment.
Where are we specifying that we have a failure while deploying in this test?
There was a problem hiding this comment.
we are not specifying/defining the result nor the success.. thus it's a failure
| @@ -569,62 +569,6 @@ describe('Base Deploy Retrieve Commands', () => { | |||
| expect(appendLineStub.calledOnce).to.equal(true); | |||
| expect(appendLineStub.firstCall.args[0]).to.equal(expectedOutput); | |||
| }); | |||
|
|
|||
| xit('should report any diagnostics if deploy failed', async () => { | |||
There was a problem hiding this comment.
Are we trying to test the same behavior in 'should unlock queue on failure'?
There was a problem hiding this comment.
I just realized I was missing a case. Thanks!
| expect(deployRetrieveExecutorClearSpy).toHaveBeenCalled(); | ||
| expect(sfdxCommandletExecutorClearSpy).toHaveBeenCalled(); | ||
| expect(unlockSpy).toHaveBeenCalled(); | ||
| expect(mockUnlock).toHaveBeenCalled(); |
There was a problem hiding this comment.
mockUnlock is returnedValue of unlockSpy. Do we need this expect statement? Also mockUnlock should be a boolean, right?
| ); | ||
| }); | ||
|
|
||
| describe('postOperation', () => { |
There was a problem hiding this comment.
Separate describe block for postOperation! Good Work on keeping the separation of concerns in mind! 🎉
There was a problem hiding this comment.
QE Notes :
✅ With Prefer Deploy on save setting On, It runs the Deploy command after every save.
✅ Errors are disappearing after successful deploy
✅ Errors are disappearing after successful push
✅ With Prefer deploy on Save setting OFF, It runs push after every save.
What does this PR do?
Makes push on save work as expected (push every time you save) and fixes issue with push errors not being cleared from the problems panel after a successful push
What issues does this PR fix or reference?
#4942, @ W-13749253@
Functionality Before
Functionality After