Updating next interval calculator file #180
Conversation
spiderjako
force-pushed
the
next-interval-calculator-update
branch
from
d0beeb1 to
deb2f46
Compare
| * @returns {object} The corresponding interval | ||
| */ | ||
| function nextIntervalCalculator(worker) { | ||
| worker.sleepTimeMsec = 0; |
There was a problem hiding this comment.
Maybe better leave the setting of the timers in a single place - the isNewBlockAvailable. Setting it here is in effect setting it every time there is a new block.
// Advanced discussion
Do we want to set it to 0 every time there is a new block or only when, no when we were so far behind that we do not even asked the node. This is a bit tricky, let's discuss in DM.
| if (!firstNewBlockCheck) { | ||
| // On the previous cycle we closed the gap to the head of the blockchain. | ||
| // Check if there are new blocks now. | ||
| async function isNewBlockAvailable(worker) { |
There was a problem hiding this comment.
I would choose another name to try to denote that this function has side effects (setting sleep timers), isX functions are usually simple checkers which return true/false. Maybe discoverNewBlocks, just to hint that something else is happening. Still naming is hard and subjective so your call.
There was a problem hiding this comment.
I tried but I'm not good at thinking of names :D Thanks for the idea
| * fromBlock: Integer, | ||
| * toBlock: Integer | ||
| * } | ||
| * If the exporter's caught up, we check for a new block. We then check whether the Node |
There was a problem hiding this comment.
This comment is confusing to me. I would instead put the comments internally to describe the if-else logic, close to the code itself, how it was before. Still subjective I guess.
There was a problem hiding this comment.
I'll try and clear up the comment a bit, but I'm not a fan of comments inside the code. If I'm unable to do that though, I'll resort to making use of them again
spiderjako
force-pushed
the
next-interval-calculator-update
branch
2 times, most recently
from
e07778a to
b3ba6d3
Compare
spiderjako
force-pushed
the
next-interval-calculator-update
branch
from
b3ba6d3 to
08c9dfd
Compare
spiderjako
force-pushed
the
next-interval-calculator-update
branch
from
08c9dfd to
000d05d
Compare
spiderjako
force-pushed
the
next-interval-calculator-update
branch
from
000d05d to
641e968
Compare
|
|
||
| const contextSecond = await analyzeWorkerContext(worker); | ||
| setWorkerSleepTime(worker, contextSecond); | ||
| assert.deepStrictEqual(contextSecond, 2); |
There was a problem hiding this comment.
Use the const values and not the magic numbers here
| const worker = new eth_worker.worker(constants); | ||
| worker.web3Wrapper = new MockIsCalledWeb3Wrapper(); | ||
| describe('nextIntervalCalculator', () => { | ||
| it('returns interval according to the worker progress', () => { |
There was a problem hiding this comment.
Isn't 'block interval is not exceeded' a better name for this test. The current one is a bit vague.
| assert.deepStrictEqual(toBlock, 100); | ||
| }); | ||
|
|
||
| it('returns the interval accordingly when lastConfirmedBlock is close', () => { |
There was a problem hiding this comment.
Is this the previously called: 'Exporter should not wait for full BLOCK_INTERVAL' test? Don't you find the previous more describing of exactly what is happening?
There was a problem hiding this comment.
I'm not sure, the waiting bit implies something else IMO. But I'll make it more clear, I don't know why I've named them in such a mysterious fashion
| assert.deepStrictEqual(worker.sleepTimeMsec, 0); | ||
| }); | ||
|
|
||
| it('Node not called if Node is ahead', async function () { |
There was a problem hiding this comment.
This test seems to be missing, don't you see value in it?
There was a problem hiding this comment.
I think it's covered with the context 0 test. Didn't really see a need to test just this line. I could, if you insist
spiderjako
force-pushed
the
next-interval-calculator-update
branch
from
641e968 to
9bc572f
Compare
spiderjako
force-pushed
the
next-interval-calculator-update
branch
from
9bc572f to
258470d
Compare
|
|
||
| }); | ||
| assert.deepStrictEqual(worker.sleepTimeMsec, 0); |
There was a problem hiding this comment.
use const values instead of 0
| worker.web3Wrapper = new MockWeb3Wrapper(4); | ||
| it('sets sleep time accordingly when we\'ve caught up to lastConfirmedBlock, but not the blockchain node head', () => { | ||
| const worker = new eth_worker.worker(constants); | ||
| setWorkerSleepTime(worker, 1); |
| assert.deepStrictEqual(worker.sleepTimeMsec, constants.LOOP_INTERVAL_CURRENT_MODE_SEC * 1000); | ||
| it('sets sleep time accordingly when we\'ve caught up', () => { | ||
| const worker = new eth_worker.worker(constants); | ||
| setWorkerSleepTime(worker, 2); |
|
|
||
| it('Block interval is not exceeded', async function () { | ||
| const worker = new eth_worker.worker(constants); | ||
| it('interval is correct if the blockchain node is ahead', async () => { |
There was a problem hiding this comment.
This test seems to cover functionality that has previously been tested separately in:
it('returns "work no sleep" case', async ()
and then
it('would not return full BLOCK_INTERVAL', ()
no need to have it if it adds nothing new
|
|
||
| it('Node gets called if Node is not ahead', async function () { |
There was a problem hiding this comment.
This test is similar to the one below and tests implementation. Maybe it makes sense to restore it.
spiderjako
force-pushed
the
next-interval-calculator-update
branch
from
258470d to
9744a5d
Compare
Description
In another PR, Yordan proposed to reform my changes in that same PR a bit and I figured that I could take those changes into a separate PR