-
Notifications
You must be signed in to change notification settings - Fork 98
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
Implement action test suite helper #1139
Conversation
chain/action_test_helpers.go
Outdated
} | ||
} | ||
|
||
require.Equal(output, test.ExpectedOutputs) |
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.
err and output should be mutually exclusive but it doesn't hurt to still check it at the end, even if err != nil
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.
Now using errors.Is
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.
You won't reach the output check because you're using require
; swapping to assert
will use t.Errorf()
under the hood instead of t.Fatalf()
, but I don't think you should.
it doesn't hurt to still check it at the end, even if
err != nil
I think it does actually, even if very minor, because it forces every error path to have a strict return value (presumably nil
) when in reality the value is undefined.
More context should anyone be interested.
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.
Long live a healthy combination of assert
and require
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 should keep using require
, otherwise the test author needs to keep thinking about which failures will cause the test to fail vs where the test can continue.
If the test fails, then debugging is needed and often the first point of unexpected behavior is what is most useful for debugging.
Additionally, require is more concise than if err != nil
style checks, which is needed to use t.Fatal
.
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 should keep using require, otherwise the test author needs to keep thinking about which failures will cause the test to fail vs where the test can continue.
That's actually the point. It avoids whack-a-mole fixes where you get partial information about problems. The test author really should be thinking about that because they're the one person with the greatest insight.
My rule of thumb is that errors use require
because that almost certainly guarantees that all other returned values are incorrect. Incorrect actual
or got
values, however, are typically more suited to assert
.
Additionally, require is more concise than if err != nil style checks, which is needed to use t.Fatal.
require
is equivalent to t.Fatal
so there's no need for the err != nil
check. The alternative is assert
, which is equivalent to t.Error
.
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.
How would we know that we have a problem of partial information? I haven't heard folks complaining about it, but maybe we're just inured...
I think that in most cases the use of testify with its rich set of assertions (and correspondingly useful contextual output on failure) obviates most of the concerns expressed in the style guide. Where those assertions aren't sufficient, we're always free to implement custom checks.
If we did decide to mix assert and require, there are cases where a group of assertion calls would need a subsequent check to ensure that any of the assertion calls failing would subsequently fail the test e.g.
mySlice, err := myFunc(...)
require.Error(err) // No point in checking the value on error
// bunch of assert.* calls
// fail the test if any of the previous assert.* calls failed
// Subsequent calls that depended on the correctness of mySlice
Maybe include integration with existing test suites in this PR? I don't think its ideal to have to review something without also getting to see how it is used. |
Absolutely makes sense ! Thanks :) |
1f01f9b
to
96a8d0a
Compare
I'm not satisfied with the way we do storage in tests, the map should not be accessed in this "raw" way. For instance, if we had a test-only in-memory database that implements |
Implemented an |
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.
Co-authored-by: aaronbuchwald <aaron.buchwald56@gmail.com> Signed-off-by: Franfran <51274081+iFrostizz@users.noreply.github.com>
* refactor: run tests individually to avoid `Context` pollution * refactor: pipe `Contexts` where possible The functions that are immediately run at declaration don't have a natural means of accepting a `Context` so can be considered the same as the `TestXXX()` entrypoint function.
func NewInMemoryStore() *InMemoryStore { | ||
return &InMemoryStore{ | ||
Storage: make(map[string][]byte), | ||
} | ||
} |
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.
We could have modified the API such that these methods never return an error because all of them cannot really fail, in the case of GetValue
then the check of nil
could have just been used for the value but it's a bad idea for an API that tries to mimic other databases so it "simulates" errors.
Co-authored-by: aaronbuchwald <aaron.buchwald56@gmail.com> Signed-off-by: Franfran <51274081+iFrostizz@users.noreply.github.com>
Co-authored-by: aaronbuchwald <aaron.buchwald56@gmail.com> Signed-off-by: Franfran <51274081+iFrostizz@users.noreply.github.com>
Closes #1138
Create an action test suite helper that would be integrated into MorpheusVM and TokenVM and could be used for VM authors to simplify the parametrized testing setup.