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.

Sign up for GitHub

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

Jump to bottom

[Travis CI] Azure PR Bot CI job must fail on the JSON.parse error from linter output #1792

Closed
wants to merge 1 commit into from
Closed

[Travis CI] Azure PR Bot CI job must fail on the JSON.parse error from linter output #1792

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 3 additions & 12 deletions scripts/momentOfTruth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,18 +74,9 @@ async function getLinterResult(swaggerPath) {
let resultString = stderr;
if (resultString.indexOf('{') !== -1) {
resultString = "[" + resultString.substring(resultString.indexOf('{')).trim().replace(/\}\n\{/g, "},\n{") + "]";
//console.log('>>>>>> Trimmed Result...');
//console.log(resultString);
try {
jsonResult = JSON.parse(resultString);
//console.log('>>>>>> Parsed Result...');
//console.dir(resultObject, {depth: null, colors: true});
return jsonResult;
} catch (e) {
console.log(`An error occurred while executing JSON.parse() on the linter output for ${swaggerPath}:`);
console.dir(resultString);
console.dir(e, { depth: null, colors: true });
}
// Do not catch the JSON parse error
jsonResult = JSON.parse(resultString);
Copy link
Contributor

Choose a reason for hiding this comment

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

what will happen when this throws? will the message in the PR just not show up? with the job showing a failure?
From the bot output perspective is the exception readable? or would it be better to keep the message you had and either re-throw the exception or try to update the exit code of the script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point..With this changes

  1. The CI Job would be red
  2. It shows something like https://travis-ci.org/Azure/azure-rest-api-specs/jobs/282957467#L590 in the log
  3. If we would prefer to catch and re-throw there'd be one more line like https://travis-ci.org/Azure/azure-rest-api-specs/jobs/282957467#L588

We can catch and re-throw if you think that'd be better on the CI. The only thought I was having was on re-throw, what'd be the best way to make this visible :) What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@vishrutshah thanks!
#1 Yes, great, that's what we want for the bot job.
Between #2 and #3 I think it'd choose #3, reason is because the error can get lost, so having it write the sentence that you purposefully chose may be helpful.
I guess, let's try and see, but we want at least #1 :)

return jsonResult;
}
return [];
};
Expand Down