Conversation
d4ef552 to
cf4d0cb
Compare
caa31d7 to
239198d
Compare
gslaughl
left a comment
There was a problem hiding this comment.
LGTM! Although it might be worth adding an integration test (despite the fact that it would be commented out), just so we can easily refer back to it later if we ever do find an event.
731c44f to
22aed83
Compare
22aed83 to
30fd141
Compare
| header, headerErr := persistHeader(db, blockNumber, blockChain) | ||
| Expect(headerErr).NotTo(HaveOccurred()) | ||
|
|
||
| // TODO: fetch event from blockchain once one exists |
There was a problem hiding this comment.
I'd be a fan of marking this test as pending if we don't have an actual event to lookup from the chain, otherwise this is basically just reproducing the transformer test, right?
There was a problem hiding this comment.
I think the main difference is that the transformer test doesn't interact with the DB at all-- it still passes even if there's no migration adding these events' table. Personally I'm indifferent whether we mark the whole test pending or have a 'partial' test like this one, just thought it was worth pointing out.
There was a problem hiding this comment.
ah good point 👍
for that matter this probably means we don't have tests that verify inserts work for all the integration tests that are currently pending 😱
There was a problem hiding this comment.
ha ha.. yeah, I think I'm going to make this pending since we may forget to update it when its not pending.
30fd141 to
d7c815c
Compare
d7c815c to
bc4a994
Compare
No description provided.