-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat(coretesting): add test events service. #20579
Conversation
WalkthroughThe changes introduce enhanced event handling within the testing context of the core package, including the addition of an event service to manage and emit events. Function renaming in the memory database ('memDB' to 'MemKV') improves clarity, while updates to the protobuf-related encoding processes refine their consistency. A new dependency on Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant MemEventsService
participant EventManager
participant DummyContext
Tester->>+MemEventsService: Instantiate
MemEventsService->>+DummyContext: Add to Context
Tester->>DummyContext: Emit Event (protoEvent)
DummyContext->>+EventManager: Emit Event
EventManager->>DummyContext: Event Recorded
Tester->>+MemEventsService: Retrieve Events
MemEventsService->>Tester: Return List of Events
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
can you migrate a few modules to use this, provides an example of if this should be in this folder or merely in a single modules pkg
Need to add few more services, such that we can mock environment fully, then we can start migrating some modules to use this |
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.
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.
Actionable comments posted: 7
Outside diff range and nitpick comments (2)
core/testing/store.go (1)
Line range hint
24-24
: Undefined functionunwrap
.Please define or import the
unwrap
function to ensure the code can execute without errors.Tools
golangci-lint
13-13: undefined: NewMemKV (typecheck)
core/testing/context.go (1)
5-5
: Incorrect import order.Please reorder the imports according to the Golang conventions with
gci
tool:gci -w --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order core/testing/context.goAlso applies to: 8-8
Tools
golangci-lint
5-5: File is not
gci
-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order (gci)
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
core/testing/go.mod
is excluded by!**/*.mod
core/testing/go.sum
is excluded by!**/*.sum
Files selected for processing (6)
- core/testing/context.go (1 hunks)
- core/testing/event.go (1 hunks)
- core/testing/event_test.go (1 hunks)
- core/testing/memdb.go (3 hunks)
- core/testing/memdb_test.go (1 hunks)
- core/testing/store.go (1 hunks)
Files skipped from review due to trivial changes (1)
- core/testing/memdb.go
Additional context used
Path-based instructions (5)
core/testing/store.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.core/testing/context.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.core/testing/event_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"core/testing/memdb_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"core/testing/event.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
golangci-lint
core/testing/store.go
24-24: undefined: unwrap (typecheck)
13-13: undefined: NewMemKV (typecheck)
core/testing/context.go
5-5: File is not
gci
-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order (gci)
8-8: File is not
gci
-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order (gci)
33-33: func
unwrap
is unused (unused)core/testing/event_test.go
13-13: undefined: Context (typecheck)
14-14: undefined: EventsService (typecheck)
34-34: undefined: EventsService (typecheck)
core/testing/memdb_test.go
13-13: undefined: NewMemKV (typecheck)
core/testing/event.go
37-37: undefined: dummyCtx (typecheck)
25-25: undefined: unwrap (typecheck)
15-15: undefined: unwrap (typecheck)
16-16: undefined: unwrap (typecheck)
29-29: undefined: unwrap (typecheck)
32-32: undefined: unwrap (typecheck)
Additional comments not posted (1)
core/testing/event.go (1)
20-22
: Type declaration forMemEventsService
looks good.
@julienrbrt I'll add them, they're more annoying to deal with considering that then I need to add gas to KV and all of that, but working on it! |
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.
Actionable comments posted: 4
Outside diff range and nitpick comments (5)
core/testing/context.go (1)
16-18
: Add documentation for thedummyCtx
struct fields.It would improve code readability and maintainability if each field in the
dummyCtx
struct had a descriptive comment explaining its purpose and usage.orm/encoding/ormfield/duration.go (2)
19-24
: Clarify encoding logic in comments for better understanding.The comments explaining the encoding logic are quite technical. Adding examples or a more detailed explanation could help in understanding the encoding process better.
Line range hint
61-61
: Resolve undefined references and functions.Multiple references and functions such as
Reader
,durationMsgType
,timestampDurationNilBz
,encodeSeconds
,encodeNanos
,decodeSeconds
,decodeNanos
,compareInt
,timestampDurationBufferSize
,int64Codec
,int32Codec
,encodeNanosV1
, anddecodeNanosV1
are used but not defined or imported. This will cause compilation errors. Define these or ensure they are correctly imported.Also applies to: 119-119, 120-120, 163-163, 261-261, 37-37, 51-51, 58-58, 62-62, 70-70, 73-73, 98-98, 103-103, 111-111, 115-115, 164-164, 168-168, 172-172, 180-180, 184-184, 190-190, 194-194, 226-226, 238-238, 245-245, 258-258, 262-262, 272-272, 275-275, 306-306, 311-311, 319-319, 323-323
orm/encoding/ormfield/timestamp.go (2)
Line range hint
97-97
: Several undefined references toReader
detected.Please ensure that the
Reader
interface is defined or imported correctly in this file to resolve the typecheck errors.Also applies to: 121-121, 149-149, 232-232, 312-312, 381-381
Line range hint
223-223
: Multiple undefined references detected.Please ensure that
timestampMsgType
,compareInt
,int64Codec
, andint32Codec
are defined or imported correctly in this file to resolve the typecheck errors.Also applies to: 224-224, 241-241, 249-249, 259-259, 264-264, 320-320, 350-350, 355-355
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (5)
- core/testing/context.go (1 hunks)
- core/testing/event.go (1 hunks)
- core/testing/event_test.go (1 hunks)
- orm/encoding/ormfield/duration.go (1 hunks)
- orm/encoding/ormfield/timestamp.go (1 hunks)
Additional context used
Path-based instructions (5)
core/testing/context.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.core/testing/event_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"core/testing/event.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.orm/encoding/ormfield/duration.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.orm/encoding/ormfield/timestamp.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
golangci-lint
core/testing/context.go
34-34: func
unwrap
is unused (unused)core/testing/event_test.go
14-14: undefined: Context (typecheck)
15-15: undefined: EventsService (typecheck)
35-35: undefined: EventsService (typecheck)
core/testing/event.go
39-39: undefined: dummyCtx (typecheck)
26-26: undefined: unwrap (typecheck)
16-16: undefined: unwrap (typecheck)
17-17: undefined: unwrap (typecheck)
30-30: undefined: unwrap (typecheck)
34-34: undefined: unwrap (typecheck)
orm/encoding/ormfield/duration.go
61-61: undefined: Reader (typecheck)
119-119: undefined: durationMsgType (typecheck)
120-120: undefined: durationMsgType (typecheck)
163-163: undefined: Reader (typecheck)
261-261: undefined: Reader (typecheck)
37-37: undefined: timestampDurationNilBz (typecheck)
51-51: undefined: encodeSeconds (typecheck)
58-58: undefined: encodeNanos (typecheck)
62-62: undefined: decodeSeconds (typecheck)
70-70: undefined: durationMsgType (typecheck)
73-73: undefined: decodeNanos (typecheck)
98-98: undefined: compareInt (typecheck)
103-103: undefined: compareInt (typecheck)
111-111: undefined: timestampDurationBufferSize (typecheck)
115-115: undefined: timestampDurationBufferSize (typecheck)
164-164: undefined: int64Codec (typecheck)
168-168: undefined: int32Codec (typecheck)
172-172: undefined: durationMsgType (typecheck)
180-180: undefined: int64Codec (typecheck)
184-184: undefined: int32Codec (typecheck)
190-190: undefined: compareInt (typecheck)
194-194: undefined: compareInt (typecheck)
226-226: undefined: timestampDurationNilBz (typecheck)
238-238: undefined: encodeSeconds (typecheck)
245-245: undefined: timestampZeroNanosBz (typecheck)
258-258: undefined: encodeNanosV1 (typecheck)
262-262: undefined: decodeSeconds (typecheck)
272-272: undefined: durationMsgType (typecheck)
275-275: undefined: decodeNanosV1 (typecheck)
306-306: undefined: compareInt (typecheck)
311-311: undefined: compareInt (typecheck)
319-319: undefined: timestampDurationBufferSize (typecheck)
323-323: undefined: timestampDurationBufferSize (typecheck)
orm/encoding/ormfield/timestamp.go
97-97: undefined: Reader (typecheck)
121-121: undefined: Reader (typecheck)
149-149: undefined: Reader (typecheck)
223-223: undefined: timestampMsgType (typecheck)
224-224: undefined: timestampMsgType (typecheck)
232-232: undefined: Reader (typecheck)
312-312: undefined: Reader (typecheck)
381-381: undefined: Reader (typecheck)
105-105: undefined: timestampMsgType (typecheck)
195-195: undefined: compareInt (typecheck)
200-200: undefined: compareInt (typecheck)
233-233: undefined: int64Codec (typecheck)
237-237: undefined: int32Codec (typecheck)
241-241: undefined: timestampMsgType (typecheck)
249-249: undefined: int64Codec (typecheck)
253-253: undefined: int32Codec (typecheck)
259-259: undefined: compareInt (typecheck)
264-264: undefined: compareInt (typecheck)
320-320: undefined: timestampMsgType (typecheck)
350-350: undefined: compareInt (typecheck)
355-355: undefined: compareInt (typecheck)
Additional comments not posted (2)
core/testing/event_test.go (1)
14-38
: Review the test coverage for new event service functionality.Ensure the new event service functionality is thoroughly tested, particularly the error paths and edge cases.
Tools
golangci-lint
14-14: undefined: Context (typecheck)
15-15: undefined: EventsService (typecheck)
35-35: undefined: EventsService (typecheck)
orm/encoding/ormfield/timestamp.go (1)
12-17
: Updated comments clarify the encoding process effectively.
// maps event emitted by the actor. | ||
events map[string][]event.Event | ||
// maps proto events emitted by the actor. | ||
protoEvents map[string][]protoiface.MessageV1 | ||
} | ||
|
||
func unwrap(ctx context.Context) *dummyCtx { |
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.
Remove or refactor the unused function unwrap
.
The function unwrap
is currently unused in the codebase. If it's intended for future use, consider commenting it out with a TODO note. Otherwise, it should be removed to keep the codebase clean.
Tools
golangci-lint
34-34: func
unwrap
is unused (unused)
) | ||
|
||
func TestEventsService(t *testing.T) { | ||
ctx := Context() |
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.
Ensure Context
and EventsService
are defined or imported.
The functions Context
and EventsService
are used but not defined or imported in this file. Ensure they are available to avoid runtime errors.
Also applies to: 15-15, 35-35
Tools
golangci-lint
14-14: undefined: Context (typecheck)
unwrap(ctx).events[moduleName] = nil | ||
unwrap(ctx).protoEvents[moduleName] = 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.
Resolve the undefined function unwrap
.
The function unwrap
is called multiple times but is not defined in this file or imported. This will cause runtime errors. Define this function or ensure it is correctly imported.
Also applies to: 26-26, 30-30, 34-34
Tools
golangci-lint
16-16: undefined: unwrap (typecheck)
17-17: undefined: unwrap (typecheck)
|
||
type eventManager struct { | ||
moduleName string | ||
ctx *dummyCtx |
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.
Define or import dummyCtx
.
The type dummyCtx
is used but not defined or imported in this file. This will cause compilation errors. Ensure that dummyCtx
is accessible in this context.
Tools
golangci-lint
39-39: undefined: dummyCtx (typecheck)
stores map[string]store.KVStore | ||
// maps event emitted by the actor. | ||
events map[string][]event.Event |
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.
Can we retain ordering a bit better? i.e. rather than a map how about an array of events + the module they came from
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.
@aaronc currently it is possible to only extract events from the module that emitted them. We could change that ofc if needed.
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.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
core/testing/go.sum
is excluded by!**/*.sum
Files selected for processing (1)
- core/testing/go.mod (1 hunks)
Files skipped from review due to trivial changes (1)
- core/testing/go.mod
Description
This PR adds a test event service that allows a developer to mock the event service behaviour and also be able to extract the emitted events in order to check a module's behaviour.
The PR also exposes
MemKV
which is a memory KV store that can be used for testing outside of module services.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
in the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Refactor
memDB
struct toMemKV
for better clarity.Documentation
google.protobuf.Duration
andgoogle.protobuf.Timestamp
.