Bsodenkamp allocation identities refactor #3115
Conversation
| it("errors if there are no identities", () => { | ||
| const thunk = () => | ||
| computeAllocation(immediate(5), [], credGrainView, 0); | ||
| expect(thunk).toThrowError("must have at least one identity"); | ||
| }); | ||
| it("errors if the total cred is zero", () => { | ||
| const thunk = () => | ||
| computeAllocation(immediate(5), [aid(0, [0])], credGrainView, 0); | ||
| expect(thunk).toThrowError("cred is zero"); | ||
| }); | ||
| it("errors if there's NaN or Infinity in Cred", () => { | ||
| const thunk = () => | ||
| computeAllocation(immediate(5), [aid(0, [NaN])], credGrainView, 0); | ||
| expect(thunk).toThrowError("invalid cred"); | ||
| }); | ||
| it("errors if there's inconsistent Cred lengths", () => { | ||
| const i1 = aid(0, [1]); | ||
| const i2 = aid(0, [1, 2]); |
There was a problem hiding this comment.
So I removed these checks, and added a validation routine in the CredGrainView to do some of the same checks, however those checks are not tested. To test some of we would have to create a a credGrainView with no cred, or with mismatched interval data for participants, and I'm not sure how to go about that. Thoughts?
There was a problem hiding this comment.
You can use the CredGrainView.fromJSON function to construct it manually with desired test data.
blueridger
dismissed
their stale review
probably would be good to not have test coverage decline.
|
Added a bunch of tests for malformed credGrainView objects. Committed and manual tested. |
| this.activeParticipants().map((p) => { | ||
| p.credPerInterval.map((c) => { |
There was a problem hiding this comment.
since you aren't building a data structure, .forEach would be a better fit than .map
here and below
| this.activeParticipants().map((p) => { | ||
| if (p.credPerInterval.length !== this.intervals().length) { | ||
| throw new Error(`participant cred per interval length mismatch`); | ||
| } | ||
| }); | ||
|
|
||
| this.activeParticipants().map((p) => { | ||
| if (p.grainEarnedPerInterval.length !== this.intervals().length) { | ||
| throw new Error(`participant grain per interval length mismatch`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
It'd be more efficient and concise if you placed all checks into the same loops. Not sure why the same loops are being duplicated throughout this function.
| describe("validation tests", () => { | ||
| const credGrainViewNoCred = CredGrainView.fromJSON( | ||
| JSON.parse( | ||
| '{"participants":[{"active":true,"identity":{"id":"YVZhbGlkVXVpZEF0TGFzdA", "subtype":"USER",\ |
There was a problem hiding this comment.
Non-blocking:
This works. An alternative strategy could've been to initialize a valid data structure in a beforeEach, and then modify it to be invalid for each test case. Parsing from string is kinda beneficial to smush it into fewer lines, but you could also have just initialized the "JSON" as a javascript hash and that would keep Flow typing stricter.
There was a problem hiding this comment.
Noted. I think my motivation was just to be able to see all the data at once.
|
|
||
| this.activeParticipants().map((p) => { | ||
| p.grainEarnedPerInterval.map((g) => { |
|
|
||
| this.activeParticipants().map((p) => { | ||
| p.credPerInterval.map((c) => { |
There was a problem hiding this comment.
map -> forEach
also can be deduped
Test Plan
bsodenkamp@Benjamins-MacBook-Pro cred % cat config/grain.json
{
"immediatePerWeek": 1000,
"balancedPerWeek": 5000,
"recentPerWeek": 19000,
"recentWeeklyDecayRate": 0.16,
"maxSimultaneousDistributions": 100
}
bsodenkamp@Benjamins-MacBook-Pro cred % pwd
/Users/bsodenkamp/projects/sourcecred.com/cred
bsodenkamp@Benjamins-MacBook-Pro cred % scdev grain -s -f > /tmp/scdevgrain
bsodenkamp@Benjamins-MacBook-Pro cred % scmain grain -s -f > /tmp/scmaingrain
bsodenkamp@Benjamins-MacBook-Pro cred % diff /tmp/scdevgrain /tmp/scmaingrain
bsodenkamp@Benjamins-MacBook-Pro cred %