-
Notifications
You must be signed in to change notification settings - Fork 10
[Telescope#2755] Adding more tests for createError #63
Conversation
c831d2e
to
79356c8
Compare
Added more tests. Shows how we can indirectly createError from an ES Error |
holy smokes Roxanne!!! I'm running it now. :) |
}); | ||
|
||
try { | ||
createError(503, elasticError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
were bringing back a 503 because elastic returns 404 not available correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just replicating what's being done in the query.js
that's causing the error
// query.js
router.get('/', validateQuery, async (req, res, next) => {
try {
const { text, filter, page, perPage } = req.query;
res.send(await search(text, filter, page, perPage));
} catch (error) {
console.log({ error }, 'query error');
next(createError(503, error)); // <-- crash is here
}
});
If you look below in the indirect test, there are different ways we can send in the "correct" status code.
The thing is though, ES has different status code names for their errors than the regular http errors. Even if we send the ES status code in for createError
, it'll just name it depending on the http error and not the ES error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: in a follow-up, since we expose createError
and Elastic
from Satellite, should we do something internally to have this work as expected by our users? That is, should we add to createError()
an extra case to deal with extracting the right bits from an Elastic error automatically?
Yes, I think we probably could. I think right now in Satellite there's no module for module.exports.createError = require('http-errors'); We could probably create its own module to modify it the way we want to. Based on the documentation, all of ES errors should be called
I have been looking at the list of http-errors. It doesn't seem like there's one called The question perhaps is just how much detail, or which object, we want to send in to |
I'm not sure if I have it down right. But since |
const _createError = require('http-error');
module.exports.createError = (...) => {.
// Do your stuff to fix up the err if it's from Elasticsearch...
return _createError(...);
}; |
Requested from #2755
What I did
Added more tests to
createError
What was found?
It's ElasticSearch's fault. They have errors that are structured differently than regular
http-errors
causing the failure.How to test
pnpm test