Skip to content

Conversation

@ShahzaibParacha
Copy link

@ShahzaibParacha ShahzaibParacha commented Nov 16, 2020

For #14381
debugLauncher.js throws a general and misleading error when it detects something wrong with launch.json file, this PR aims to fix that by tackling each case independently so that the correct error is produced and the user knows exactly what to fix.

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).
  • The wiki is updated with any design decisions/details.

@karrtikr karrtikr self-assigned this Nov 16, 2020
Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Don't forget to thank yourself in news entry as mentioned in the PR template🙂

traceError('could not get debug config', exc);
const appShell = this.serviceContainer.get<IApplicationShell>(IApplicationShell);
await appShell.showErrorMessage('Could not load unit test config from launch.json');
await appShell.showErrorMessage('Could not load unit test config from launch.json; missing a field');

Choose a reason for hiding this comment

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

Suggested change
await appShell.showErrorMessage('Could not load unit test config from launch.json; missing a field');
await appShell.showErrorMessage('Could not load unit test config from launch.json as it is missing a field');

Comment on lines 59 to 64
if (!Array.isArray(parsed.configurations)) {
throw Error('malformed launch.json');
}
if (!parsed.configurations) {
throw Error('Missing field in launch.json: configurations');
}

Choose a reason for hiding this comment

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

Suggested change
if (!Array.isArray(parsed.configurations)) {
throw Error('malformed launch.json');
}
if (!parsed.configurations) {
throw Error('Missing field in launch.json: configurations');
}
if (!parsed.configurations || !Array.isArray(parsed.configurations)) {
throw Error('Missing field in launch.json: configurations');
}

@ghost
Copy link

ghost commented Nov 17, 2020

CLA assistant check
All CLA requirements met.

news/14739.md Outdated
@@ -0,0 +1,2 @@
Modified the errors generated by debugLauncher.ts to be more specific when launch.json is not properly configured
Copy link

@karrtikr karrtikr Nov 17, 2020

Choose a reason for hiding this comment

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

Suggested change
Modified the errors generated by debugLauncher.ts to be more specific when launch.json is not properly configured
Modified the errors generated when `launch.json` is not properly configured to be more specific about which fields are missing.

Making the message user friendly as users have no idea about debugLauncher.ts.

Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

You need to put the news entry under news\3 Code Health. Other than that LGTM.

@ShahzaibParacha
Copy link
Author

I think everything should be good now, let me know if I can make something otherwise better.

@karrtikr
Copy link

The news entry text was not changed as I suggested.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ShahzaibParacha
Copy link
Author

The news entry text was not changed as I suggested.

Sorry, I think I missed that last night. Fixed it now!

@ShahzaibParacha
Copy link
Author

@karrtikr Thank you for guiding me through this issue, it was a very positive and fun experience. I look forward to contributing more in the coming weeks.

@karrtikr karrtikr merged commit 4f4f9b0 into microsoft:main Nov 23, 2020
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