Expose cloudwatch startTime, startTime should be in UTC #36
Conversation
…ime value to be in UTC instead of local timezone
|
@tbradyRobustWealth thanks for the pr. I'll look over the rest of the pr today |
| const { filterLogEvents } = require('../utils/cloudwatch'); | ||
| const { wrapWithRetries } = require('./utils'); | ||
|
|
||
| const error = new Error('Unknown error'); | ||
| filterLogEvents.mockReturnValue(Promise.reject(error)); | ||
|
|
||
| epochDateMinusHours.mockReturnValue(11 * 60 * 60 * 1000); |
There was a problem hiding this comment.
I think you can remove mocking epochDateMinusHours since you're passing startTime explicitly
| } | ||
|
|
||
| export const expectedProps = ['region', 'function', 'pattern']; | ||
| export const expectedProps = ['region', 'function', 'startTime', 'pattern']; |
There was a problem hiding this comment.
If startTime is optional you should not add it to expectedProps (otherwise not passing it will throw an error).
It is not visible in the test "startTime should be defaulted when not passed in" since the common module is mocked thus verifyProps doesn't throw.
If you apply the mock as follows:
jest.mock('../common', () => {
const actual = jest.requireActual('../common');
return { ...actual, epochDateMinusHours: jest.fn() };
});
The test will fail
| @@ -98,6 +98,7 @@ expect.assertions(1); // makes sure the assertion was called | |||
| await expect({ | |||
| region: 'us-east-1', | |||
| function: 'functionName', | |||
| startTime: 0 /* optional (millis since epoch in UTC, defaults to now-1 hour) */, | |||
There was a problem hiding this comment.
You should add this line under chai/README.md as well
| const { filterLogEvents } = require('../utils/cloudwatch'); | ||
|
|
||
| const error = new Error('Unknown error'); | ||
| filterLogEvents.mockReturnValue(Promise.reject(error)); | ||
|
|
||
| epochDateMinusHours.mockReturnValue(11 * 60 * 60 * 1000); |
There was a problem hiding this comment.
I think you can remove mocking epochDateMinusHours since you're passing startTime explicitly
|
|
||
| export const epochDateMinusHours = (hours: number) => { | ||
| const now = new Date(); | ||
| return Date.parse(now.toUTCString()) - hoursToMilliseconds(hours); |
There was a problem hiding this comment.
How is this different from return Date.now() - hoursToMilliseconds(hours);
Please see the gist here: https://gist.github.com/erezrokah/3fbe380840223a82fde16a16976a0908
The only notable difference I could see is that with Date.parse(now.toUTCString()) the milliseconds data is lost
This PR is intended to give people the ability to pass in a startTime to the toHaveLog method. This gives users of the library more control of the time frame they are searching.
Also, there is a small bug fix in how the default cloudwatch startTime was calculated. It should be in UTC time, rather than local time.