Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

05 stable update tests #226

Merged
merged 6 commits into from
Nov 4, 2019

Conversation

samparsky
Copy link
Contributor

Fix #223

Copy link
Member

@Ivshti Ivshti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great job, amazing!

But there's a few things that need to be addressed

@@ -37,7 +36,7 @@ function aggrAndTick() {
}

tape('submit events and ensure they are accounted for', async function(t) {
const evs = genEvents(3).concat(genEvents(2, 'anotherPublisher'))
const evs = genEvents(3).concat(genEvents(2, '0x48b6d5748885988cb657a06647c05fc2af1ffacf'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe put this in the dummy data constants (dummyVals)

@@ -270,7 +261,7 @@ async function testRejectState(t, expectedReason, makeNewState) {
tape('RejectState: wrong signature (InvalidSignature)', async function(t) {
await testRejectState(t, 'InvalidSignature', function(newState) {
// increase the balance, so we effectively end up with a new state
const balances = { ...newState.balances, someoneElse: '1' }
const balances = { ...newState.balances, '0x033ed90e0fec3f3ea1c9b005c724d704501e0196': '1' }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, consider putting this in the dummyVals but I have an important question here:

how do we make sure that 0x033 does not override something that's already in newState.balances?

also, in the second case, how do we ensure that '0x033' was not previously present in the newState.balances
how about generating a random addr in the second case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now that this is the creator from dummyVals

I think that you should indeed gen a random addr here, in this test and in the next (const fakeBalances=)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah true, I have added random address generation

test/lib.js Outdated
return Promise.all([exec(leaderTick), exec(followerTick)])
}

function nonce() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a more elegant way to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the change to use current timestamp as nonce

@@ -142,11 +142,7 @@ tape('POST /channel: should not work with invalid withdrawPeriodStart', async fu
...dummyVals.channel.spec,
withdrawPeriodStart: new Date('2200-01-01').getTime(),
minPerImpression: '1',
maxPerImpression: '1',
validators: [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be removed because it was already in the dummyVals.channel.spec, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

`./bin/validatorWorker.js --single-tick --adapter=dummy --dummyIdentity=awesomeFollower --sentryUrl=http://localhost:8006`
)
])
let leaderTick = `./bin/validatorWorker.js --single-tick --adapter=dummy --dummyIdentity=0xce07cbb7e054514d590a0262c93070d838bfba2e --sentryUrl=http://localhost:8005`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we avoid hardcoding those here..we have dummyVals

if (process.env.RUST_VALIDATOR_WORKER) {
leaderTick = `RUST_BACKTRACE=1 ${
process.env.RUST_VALIDATOR_WORKER
} -a dummy -i 0xce07cbb7e054514d590a0262c93070d838bfba2e -u http://localhost:8005 -t`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we avoid hardcoding those here..we have dummyVals

@Ivshti Ivshti merged commit 34a665a into AmbireTech:05-stable Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tests: Update tests to enable compatibility with Rust validator_worker
2 participants