-
Notifications
You must be signed in to change notification settings - Fork 14
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
Delete migrated file 400_contributeAndCallback.js #177
Delete migrated file 400_contributeAndCallback.js #177
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #177 +/- ##
===========================================
- Coverage 84.87% 84.50% -0.37%
===========================================
Files 35 35
Lines 1084 1084
Branches 221 221
===========================================
- Hits 920 916 -4
- Misses 164 168 +4 ☔ View full report in Codecov by Sentry. |
…ture/400_contribute-and-callback-cleaning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
expect(deal.trust).to.equal(1); | ||
expect(deal.category).to.equal(0); | ||
expect(deal.tag).to.equal(HashZero); // Standard | ||
expect(deal.requester).to.equal(requester.address); | ||
expect(deal.beneficiary).to.equal(AddressZero); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not:
expect(deal.trust).to.equal(1); | |
expect(deal.category).to.equal(0); | |
expect(deal.tag).to.equal(HashZero); // Standard | |
expect(deal.requester).to.equal(requester.address); | |
expect(deal.beneficiary).to.equal(AddressZero); | |
expect(deal.trust).to.equal(1); | |
expect(deal.category).to.equal(2); | |
expect(deal.tag).to.equal(0x..3); // Standard | |
expect(deal.requester).to.equal(requester.address); | |
expect(deal.beneficiary).to.equal(0x..4); | |
[...] |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My goal is to use minimal setup code and check that all fields of the deal are returned and not necessarily go into more details because we already have other tests that check all relevant fields for each type of deal.
expect(contribution.resultHash.length).to.equal(66); | ||
expect(contribution.resultSeal.length).to.equal(66); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To understand, why checking length instead of content?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to the reduce noise in the test and not having to worry about the hashes. I assert that a hash is returned without necessarily checking its content as other tests (contribution tests) are already doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you :)
No description provided.