Bug/etcm 636 [!pr]#925
Conversation
|
Is this PR dependant on #925 being merged? Meaning that you will have to update the tests? |
mostly that's a separate effort here of investigating the underlying causes of that none.get situation. But the tests will be impacted a bit, given that the getBestBlock will return option after Bogdan's PR is merged. |
![hackerone-code[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network.
@AnastasiiaL you can click here to see the review status or cancel the code review job - or - cancel by adding [!pr] to the title of the pull request.
There was a problem hiding this comment.
This pull request would normally be cancelled because the title contains "[!pr]"; however since the review was requested manually from the PullRequest dashboard, it was sent to our network to be reviewed. If you wish to cancel PullRequest review, you can do so manually from the PullRequest dashboard.
AnastasiiaL
force-pushed
the
bug/ETCM-636
branch
from
1339a06 to
499fe54
Compare
There was a problem hiding this comment.
⚠️ Warning
PullRequest detected a force-push on this branch. This may have caused some information to be lost, and additional time may be required to complete review of the code. Read More
There was a problem hiding this comment.
This looks reasonable overall. There were two main themes to my comments:
- Repeated
chackpointtypo - Trying to make the tests more readable. This includes:
- Shortening the setup. Boilerplate tends to hide bugs.
- Emphasizing what differentiates some seemingly similar tests.
- Perhaps improving some of the prose test descriptions? Some didn't seem to obviously correspond to the actual code (e.g. not sure about the 'from morpho' part and the 'and back' part of 'longer chain to a shorter one and back' wasn't clear to me)
- Trying to clarify the state of the blockchain at important times. There's a lot of updating of the chain and which implicitly asks the reader to track chain state in their head.
However, I don't have full context on this area so the comments in the second bullet here may be off the mark.
Reviewed with ❤️ by PullRequest
ten15bit
left a comment
ten15bit
left a comment
There was a problem hiding this comment.
👍 Only left some minor comments. Let's have a quick call today and then it's ✔️ from my side.
AnastasiiaL
force-pushed
the
bug/ETCM-636
branch
from
efa2d02 to
c866c95
Compare
4 participants
Description
None.get on bestBlock happened because of the inconsistency between cache and db. This PR introduces uni and integration tests that try to reproduce the issue. In case of it test - it's not possible to reproduce as probable cause is the thread of execution dying in the middle of updates.
Also updating one method with preferred order of execution - first saving to cache, after to db.