update errorHandler templates by loay · Pull Request #285 · strongloop/loopback-workspace

loay · 2016-06-09T21:19:40Z

connect to https://github.com/strongloop-internal/scrum-loopback/issues/896

loay · 2016-06-10T02:01:53Z

@slnode test please

loay · 2016-06-10T08:21:58Z

@bajtos PTAL
I closed the previous PR: #284 as it seems to be corrupted. Let's use this one instead please. Thanks.

bajtos · 2016-06-10T13:00:07Z

server/config.json

    "cors": false,
    "errorHandler": {
-      "disableStackTrace": false
+      "debug": false,


disableStackTrace:false should be converted into debug:true.

bajtos · 2016-06-10T13:06:12Z

@loay reviewed, PTAL

loay · 2016-06-12T10:27:57Z

@bajtos PTAL

bajtos · 2016-06-13T08:45:55Z

test/end-to-end.js

 var given = require('./helpers/given');
 var should = require('chai').should();
+var loopback = require('loopback');
+var boot = require('loopback-boot');


Hmm, I think this may not work when the scaffolded application uses a different loopback or loopback-boot version than we use in loopback-workspace. Considering that fix is not straightforward and the situation may actually never happen, I think it's ok to leave these two lines as they are, but please add a comment pointing out this possible problem.

loay · 2016-06-14T07:02:15Z

updated test cases and rebased.
PTAL @bajtos

bajtos · 2016-06-14T12:39:03Z

test/end-to-end.js

 var ref = TestDataBuilder.ref;
 var given = require('./helpers/given');
 var should = require('chai').should();
+// this may not work when the scaffolded application uses a different


// This...

to make this consistent with the third line of the comment, which starts with capital letter too

bajtos · 2016-06-15T10:56:15Z

@loay sorry for the back and forth, I realised my advise about checking the missing stack trace was wrong :(

loay · 2016-06-16T08:09:49Z

@bajtos
Now worries. It is just that res.body is giving an empty object, so I think that's something I need to figure out. Also, res.error works fine and it is giving stack in both dev and production !

loay · 2016-06-16T21:19:07Z

@bajtos PTAL

bajtos · 2016-06-17T11:36:15Z

test/end-to-end.js

+              .end(function(err, res) {
+                app.use(function(err, res) {
+                  var responseBody = JSON.stringify(res.body);
+                  console.log(responseBody);


Forgotten console.log. If you find it useful to have a log around, then please use debug instead.

In case debug is not defined in this file yet: in tests, we usually define debug at the top of the test file this way:

var debug = require('debug')('test');

bajtos · 2016-06-17T11:56:58Z

Now worries. It is just that res.body is giving an empty object, so I think that's something I need to figure out. Also, res.error works fine and it is giving stack in both dev and production !

From what I can tell, it looks like the application is misconfigured, strong-error-handler is not installed and the response contains empty body because it's produced by express default error handler.

Debug log:

  (...)
  superagent get http://127.0.0.1:60993/url-does-not-exist +22ms
  express:router dispatching GET /url-does-not-exist +20ms
  express:router query  : /url-does-not-exist +3ms
  express:router expressInit  : /url-does-not-exist +1ms
  finalhandler default 404 +3ms
  superagent get http://127.0.0.1:60993/url-does-not-exist -> 404 +12ms
  superagent end get http://127.0.0.1:60993/url-does-not-exist +6ms

I don't think there should be finalhandler entry when the request is handled by a middleware (like strong-error-handler).

Additionally, if you look at res.error, it contains the stack trace from the client side, not the server error.

   { Error: cannot GET /url-does-not-exist (404)
       at Response.toError (/Users/bajtos/src/loopback/workspace/node_modules/superagent/lib/node/response.js:106:13)
       at Response.setStatusProperties (/Users/bajtos/src/loopback/workspace/node_modules/superagent/lib/node/response.js:183:12)
       at new Response (/Users/bajtos/src/loopback/workspace/node_modules/superagent/lib/node/response.js:42:8)
       at IncomingMessage.<anonymous> (/Users/bajtos/src/loopback/workspace/node_modules/superagent/lib/node/index.js:918:22)
       at emitNone (events.js:91:20)
       at IncomingMessage.emit (events.js:185:7)
       at endReadableNT (_stream_readable.js:926:12)
       at _combinedTickCallback (internal/process/next_tick.js:74:11)
       at process._tickDomainCallback (internal/process/next_tick.js:122:9)
       at process.fallback (/Users/bajtos/src/loopback/workspace/node_modules/async-listener/index.js:450:15)
     status: 404,
     text: 'Cannot GET /url-does-not-exist\n',
     method: 'GET',
     path: '/url-does-not-exist' },

@loay Since this change is not urgent, I am going to use this as a learning opportunity for you. Here is what I'd like you to do:

Run only the first test from the new tests you are adding, for example by running

$ mocha -g 'dev/prod.*develo'`

(There are other ways, pick whatever works best for you. Just make sure to include only tests that use the same sandbox as the test you are writing.)

After mocha finishes, you will have the scaffolded application left in test/sandbox directory. You can run node . there to start the app and call its REST API directly.

Right now, it fails with the following error:

$ node .
module.js:561
    throw err;
    ^

SyntaxError: /Users/bajtos/src/loopback/workspace/test/sandbox/server/middleware.development.json: Unexpected end of JSON input

Do not fix this error yet!

This has discovered a problem in your test - how comes the malformed JSON did not fail the test with? Find out the answer and fix this problem first, before making any changes in the templates. You should fix the problem in all your new tests that are affected.
Now it's time to fix that JSON template file (you are missing a closing curly brace).
After you get your tests to pass with the new implementation in place, make sure they also fail when templates change (a regression occurs). For example, you can temporarily switch debug to the opposite value (debug:false in dev, debug:true in production), and verify that the tests are failing with a descriptive error message pointing out to this problem.

loay · 2016-06-22T15:30:17Z

@bajtos PTAL. Thanks.

loay added the #review label Jun 9, 2016

loay assigned bajtos Jun 10, 2016

bajtos reviewed Jun 10, 2016
View reviewed changes

bajtos assigned loay and unassigned bajtos Jun 10, 2016

loay assigned bajtos and unassigned bajtos and loay Jun 12, 2016

bajtos reviewed Jun 13, 2016
View reviewed changes

bajtos assigned loay and unassigned bajtos Jun 13, 2016

loay force-pushed the 2strongHandler branch from d6e41d6 to c0da53d Compare June 14, 2016 07:01

loay assigned bajtos and unassigned loay Jun 14, 2016

bajtos reviewed Jun 14, 2016
View reviewed changes

loay assigned bajtos and unassigned loay Jun 16, 2016

bajtos reviewed Jun 17, 2016
View reviewed changes

loay assigned loay and bajtos and unassigned bajtos and loay Jun 22, 2016

loay force-pushed the 2strongHandler branch 2 times, most recently from 0863f2c to 7143d31 Compare June 22, 2016 17:26

update test cases-fix server path

afb4b90

loay assigned loay and unassigned bajtos Jun 22, 2016

loay force-pushed the 2strongHandler branch 2 times, most recently from 0863f2c to 81e3867 Compare June 22, 2016 19:19

loay closed this Jun 23, 2016

loay force-pushed the 2strongHandler branch from 81e3867 to c1e3689 Compare June 23, 2016 04:44

loay removed the #review label Jun 23, 2016

loay reopened this Jun 23, 2016

loay added the #review label Jun 23, 2016

loay closed this Jun 23, 2016

loay removed the #review label Jun 23, 2016

loay mentioned this pull request Jun 23, 2016

Add strong-error-handler to project templates #292

Merged

bajtos deleted the 2strongHandler branch September 10, 2020 14:00

Conversation

loay commented Jun 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loay commented Jun 10, 2016

Uh oh!

loay commented Jun 10, 2016

Uh oh!

bajtos Jun 10, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jun 10, 2016

Uh oh!

loay commented Jun 12, 2016

Uh oh!

bajtos Jun 13, 2016

Choose a reason for hiding this comment

Uh oh!

loay commented Jun 14, 2016

Uh oh!

bajtos Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jun 15, 2016

Uh oh!

loay commented Jun 16, 2016

Uh oh!

loay commented Jun 16, 2016

Uh oh!

bajtos Jun 17, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jun 17, 2016

Uh oh!

loay commented Jun 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

loay commented Jun 9, 2016 •

edited

Loading