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

error-reporting: Fix report() silently failing #2363

Conversation

DominicKramer
Copy link
Contributor

If the report() method is called with a number, an associated
error never appears in the error reporting console, and no log
message is printed to the user to indicate why the error has not
appeared.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 6, 2017
@@ -4,7 +4,7 @@
"main": "./src/index.js",
"repository": "GoogleCloudPlatform/google-cloud-node",
"scripts": {
"test": "nyc --exclude=\"fuzzer.js\" mocha ./test/unit/**/*.js",
"test": "nyc --exclude=\"fuzzer.js\" mocha ./test/unit/*.js ./test/unit/**/*.js",

This comment was marked as spam.

This comment was marked as spam.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.988% when pulling 0b34534 on DominicKramer:bug/reporting-non-errors-silently-fails into 03c276d on GoogleCloudPlatform:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.988% when pulling 93a897c on DominicKramer:bug/reporting-non-errors-silently-fails into 03c276d on GoogleCloudPlatform:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.988% when pulling 10807f1 on DominicKramer:bug/reporting-non-errors-silently-fails into 03c276d on GoogleCloudPlatform:master.

@DominicKramer DominicKramer requested a review from ofrobots June 7, 2017 17:04
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.988% when pulling 2e6b422 on DominicKramer:bug/reporting-non-errors-silently-fails into 03c276d on GoogleCloudPlatform:master.

Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

Added some comments. Some of them were present in the original code, so it might be acceptable to do them in a follow-on (ie. keep this as a refactor, and make logical changes in a follow-on).

* Constructs a string representation of the stack trace at the point where
* this function was invoked. Note that the stack trace will not include any
* references to this function itself.
* @function buildStackTrace

This comment was marked as spam.

This comment was marked as spam.

var fauxError = new Error('');
var fullStack = fauxError.stack.split('\n');
var cleanedStack = fullStack.slice(2).join('\n');
var cleanedStack = buildStackTrace('', 1);

This comment was marked as spam.

This comment was marked as spam.

function populateErrorMessage(ob, em) {
if (ob === null || ob === undefined) {
populateFromUnknown(ob, em);
} else if (ob instanceof Error) {

This comment was marked as spam.

This comment was marked as spam.

* @returns {Undefined} - does not return anything
*/
function populateFromNumber(num, errorMessage) {
var message = isNumber(num) && isFunction(num.toString) ? num.toString() : '';

This comment was marked as spam.

This comment was marked as spam.

* @returns {Undefined} - does not return anything
*/
function populateFromString(str, errorMessage) {
errorMessage.setMessage(buildStackTrace(str, 3));

This comment was marked as spam.

This comment was marked as spam.

If the `report()` method is called with a number, an associated
error never appears in the error reporting console, and no log
message is printed to the user to indicate why the error has not
appeared.
The `Error.captureStackTrace` method is now used in the
`buildStackTrace` function to create a stack trace.
The code for verifiying that stack traces don't contain
error-reporting specific frames has been consolidated into a
single location.
@DominicKramer DominicKramer force-pushed the bug/reporting-non-errors-silently-fails branch from 28226ff to 3df370b Compare June 15, 2017 18:36
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.988% when pulling 3df370b on DominicKramer:bug/reporting-non-errors-silently-fails into 06e743e on GoogleCloudPlatform:master.

In particular, the `buildStackTrace` was updated to ensure that
no error-reporting specific frames are included in the built
stack trace.  In addition, the system-tests have been updated to
ensure error-reporting related frames, and only those frames, are
removed from the built stack trace.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.988% when pulling 24fbc3b on DominicKramer:bug/reporting-non-errors-silently-fails into 06e743e on GoogleCloudPlatform:master.

@DominicKramer
Copy link
Contributor Author

@stephenplusplus This is ready to land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: error-reporting cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants