Swallows errors on ml lookups that we know might fail#25783
Swallows errors on ml lookups that we know might fail#25783jasonrhodes merged 2 commits intoelastic:masterfrom
Conversation
💚 Build Succeeded |
| } | ||
| throw e; | ||
| // swallow error because there are lots of reasons | ||
| // the ml index lookup may fail |
There was a problem hiding this comment.
I'm split on leaving it like this, or changing it to:
if (e.statusCode >= 400 && e.statusCode < 500) {
return null;
}
throw e;
There was a problem hiding this comment.
Yeah, blindly swallowing all errors might be a bit much. I think I like your suggestion with only swallowing 4xx errors.
A middle ground could be swallowing all http errors:
if (e.statusCode > 0) {
return null;
}
throw e;
| Promise.reject(new Error('anomaly lookup failed')) | ||
| ); | ||
|
|
||
| expect(getAnomalyAggs({ client: failClient })).resolves.toEqual(null); |
There was a problem hiding this comment.
Async jest tests are crazy deceiving. You need to return or await your expectation when doing this.
| expect(getAnomalyAggs({ client: failClient })).resolves.toEqual(null); | |
| return expect(getAnomalyAggs({ client: failClient })).resolves.toEqual(null); |
If you don't return/await the test will always pass regardless. Eg. This tests should not pass but it does:
const failClient = jest.fn(() => Promise.resolve('I am for sure not null'));
expect(getAnomalyAggs({ client: failClient })).resolves.toEqual(null);There was a problem hiding this comment.
Ahhh yes, I used to be so good about this but switched to async/await for all my tests and then forgot about it. Will fix, good catch.
|
All comments made |
|
@sqren this should be all set now |
💚 Build Succeeded |
* Swallows errors on ml lookups that we know can fail * Adjusts when we swallow ml lookup errors, fixes async test
3 participants
Closes #23463
Summary
Updated this function to swallow its own errors, and not just 404 status code errors. (Still deciding if this should swallow all or just 4xx errors...)
Adds a test as well.