Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: find server errors and display them with FileResponse errors #210

Merged
merged 3 commits into from
Sep 22, 2021

Conversation

WillieRuemmele
Copy link
Contributor

What does this PR do?

checks the server response for other issues not present in the getFileResponse() method from SDR and displays them alongside other deployment issues.

What issues does this PR fix or reference?

@W-9816657@ forcedotcom/cli#1180

OLD

 ➜  sfdx force:source:deploy -m CustomField:Account.Name 
*** Deploying with SOAP API ***
Deploy ID: 0AfJ000001m2PoWKAU
SOURCE PROGRESS | ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ | 0/1 Components
ERROR running force:source:deploy:  Deploy failed.

NEW

 ➜  sfdx force:source:deploy -m CustomField:Account.Name 
*** Deploying with SOAP API ***
Deploy ID: 0AfJ000001m1diRKAQ
SOURCE PROGRESS | ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ | 0/1 Components

=== Component Failures [1]
Type   Name             Problem
─────  ───────────────  ──────────────────
Error  Account.NameNew  Not in package.xml

@WillieRuemmele WillieRuemmele requested review from shetzel, mshanemc and a team September 20, 2021 21:13
expect(tableStub.calledTwice).to.equal(true);
expect(styledHeaderStub.firstCall.args[0]).to.contain('Test Failures [1]');
expect(styledHeaderStub.secondCall.args[0]).to.contain('Apex Code Coverage');
expect(styledHeaderStub.calledThrice).to.equal(true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these tests were giving false positives and needed to be updated

}

if (this.result?.response?.details?.componentFailures) {
const deployMessages = toArray(this.result.response.details.componentFailures);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

failures.push(...fileResponses);
}

if (this.result?.response?.details?.componentFailures) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aren't componentFailures and fileResponses going to include some of the same errors, and so this would show duplicates?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ugh. dupes 🤦

@mshanemc
Copy link
Contributor

if the dupes aren't happening, looks good.

const deployMessages = toArray(this.result.response.details.componentFailures);
const deployMessages = toArray(this.result?.response?.details?.componentFailures);
if (deployMessages.length > failures.length) {
// if there's additional failures in the API response, find the failure and add it to the output
deployMessages.map((deployMessage) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could get really loopy on a big deployment fail (use of find within a loop where find is doing multiple string.includes)

perf suggestion: iterate failures once to build a keyed Map< type+fullName>, deployMessage>
then, you can iterate the deployMessages once and use deployMessageMap.has( type + fullName)
No would be a good sign to add it
has=true could still compare the messages (if that's actually a source of dupes)

At the very least, do the (do type and name match?) conditional first so that it mostly exits before having to do the string.includes ops

if (this.result?.response?.details?.componentFailures) {
const deployMessages = toArray(this.result.response.details.componentFailures);
const deployMessages = toArray(this.result?.response?.details?.componentFailures);
if (deployMessages.length > failures.length) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea

@mshanemc mshanemc merged commit 99df1ca into main Sep 22, 2021
@mshanemc mshanemc deleted the wr/findServerErrors branch September 22, 2021 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants