-
Notifications
You must be signed in to change notification settings - Fork 17
Conversation
9a1745a
to
856b7e9
Compare
expect(getFromWeb).toHaveBeenCalledTimes(1); | ||
expect(getFromWeb).toHaveBeenCalledWith(apiCallPage(1)); | ||
done(); | ||
done(err); |
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.
done
allows passing the first argument as an error to cause the test to fail (which allows it to be glued in to the standard callback pattern) - in this case, we expect err
will be null
here so the test won't fail, but if err
was ever set to an error, the test would fail as there would have been an issue within the produce
function.
const NativeDate = Date; | ||
global.Date = jest.fn( | ||
input => new NativeDate(input || "2018-11-06T10:30:00") | ||
); |
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.
There was an issue with the previous stubbing of this - if we just return an existing date object, that date object can be mutated by the logic calling it, causing issues in other tests.
So instead, this stubbing now creates a new date object on each call, and defaults calls without a specific time to 6th November 2018.
producer.produce(event, context, function(err, response) { | ||
expect(response).toEqual({ | ||
message: [ | ||
"path/to/new/file.json", |
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.
NAB: this initially looked a bit odd to me as I expected the producer to dedupe the file paths but am I correct in saying this is a side effect of mocking uploadTo.mockImplementation(resolved({ key: "path/to/new/file.json" }));
and it shouldn't be an issue for producers?
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.
@apoclyps Yep, spot on - the mock for the upload function just returns a resolved promise with a response object that's similar to what we'd get back when uploading to S3. The path that's returned isn't dynamic, it's always this hard coded path.
What this test shows is that we've taken all three of these paths for where new files have been uploaded to, and combined them into a single array of paths that's been returned from the producer; this is what will end up in the lambda logs.
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.
LGTM 👍
What's Changed
Add tests for Meetup.com producer lambda, as well as updates to existing producer lambda tests to align with new changes (ordering,
err
check, naming, etc.)