Add: empty aggregates test & Admin routes#302
Conversation
samparsky
changed the title
| t.end() | ||
| }) | ||
|
|
||
| tape('validatorWorker: does not apply empty aggr', async function(t) { |
There was a problem hiding this comment.
@Ivshti here is the test that checks for that the validatorWorker doesn't apply an empty aggr
| }) | ||
|
|
||
| tape('/validator-messages: reject Accounting/NewState messages with empty balances', async function( | ||
| t |
There was a problem hiding this comment.
And here we test that the sentry never accepts newstate/accounting messages with empty balances
samparsky
force-pushed
the
fix-admin-check
branch
from
1261b6a to
775d1aa
Compare
samparsky
force-pushed
the
fix-admin-check
branch
from
4285886 to
35cd3b2
Compare
samparsky
force-pushed
the
fix-admin-check
branch
from
35cd3b2 to
1cba156
Compare
samparsky
force-pushed
the
fix-admin-check
branch
from
1cba156 to
d7434a2
Compare
Ivshti
left a comment
Ivshti
left a comment
There was a problem hiding this comment.
Looks good except a few comments
remark for the future: please use separate commits for separate changes
adapters/lib.js
Outdated
| } | ||
| // we split to remove space JSON.stringify adds | ||
| // between keys | ||
| const formattedSpec = JSON.stringify(channel.spec) |
There was a problem hiding this comment.
don't do this, since it will remove the spaces in the strings too
just count it as it is...despite the spaces
| .split(' ') | ||
| .join('') | ||
| // we use buffer instead of string length to cater for | ||
| // non utf8 strings |
| async function adminAnalytics(req) { | ||
| const { channels, earner } = req.query | ||
| if (!channels) throw new Error('please provide channels query param') | ||
| return analytics(req, channels.split('+'), earner) |
There was a problem hiding this comment.
there are two schools of thought here
you either pass multiple channels by doing ?channelId=..&channelId=... in which case express will give you an array for channel ID
or you handle it manually as you do here...
I'm fine with either - I like this one more, so this is good 👍
test/integration.js
Outdated
| events: [ | ||
| { | ||
| type: constants.eventTypes.update_targeting, | ||
| targetingRules: [ |
There was a problem hiding this comment.
you can set this to an empty array, so that it doesn't distract from the main purpose of the test
also, did you test what happens if we comment out the fix (if (eventReducer.isEmpty(aggr)) { return })?
There was a problem hiding this comment.
yeah i did it produces the a new Accounting state
2 participants
Fixes #300
Fixes #286
Fixes #285