-
Notifications
You must be signed in to change notification settings - Fork 70
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
Deal validation tests #407
Conversation
itests/dummydeal_test.go
Outdated
|
||
// deal fails because signature is invalid | ||
res, err = f.makeDummyDeal(dealUuid, carFilepath, rootCid, server.URL+"/"+filepath.Base(carFilepath), false, abi.NewTokenAmount(2000000), withInvalidSignature()) | ||
require.NoError(t, err) | ||
require.False(t, res.Accepted) | ||
require.Contains(t, res.Reason, "error verifying signature") | ||
log.Debugw("got response from MarketDummyDeal", "res", spew.Sdump(res)) |
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 think we don't really need the invalid signature and min ask tests at the dummy deal level if we're already testing these things in the provider unit test
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.
@dirkmc Dosen't hurt to have it for a sanity check as these are two important validations and these finish quickly as we don't wait for these deals to be sealed since they get rejected right off the bat.
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 think I'd rather keep all the validation testing in one place. Someone looking at this code later might think they should add validation testing code here. In general I favour having the minimal amount of tests to keep the code maintainable.
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.
Okay, done.
Closes #216.