-
Notifications
You must be signed in to change notification settings - Fork 39
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
fix(i): Tests with primary doc patch update events #2850
fix(i): Tests with primary doc patch update events #2850
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2850 +/- ##
===========================================
- Coverage 79.43% 79.39% -0.04%
===========================================
Files 323 323
Lines 24683 24681 -2
===========================================
- Hits 19606 19594 -12
- Misses 3678 3684 +6
- Partials 1399 1403 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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 very much for removing that test package Keenan, just a couple of small todo's for you before merge :)
tests/integration/events.go
Outdated
func waitForUpdateEvents( | ||
s *state, | ||
nodeID immutable.Option[int], | ||
updates map[string]struct{}, |
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.
todo: Please make it a little clearer as to what updates
is, either by renaming (preferred, perhaps to docIDs
) or documentation.
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.
renamed to docIDs
tests/integration/events.go
Outdated
s.nodeP2P[id].expectedDocHeads[evt.DocID] = evt.Cid | ||
} | ||
if i >= len(s.nodeConfigs) { | ||
continue // not testing network state |
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.
todo: This comment caused me a fair bit of confusion, and I initially thought it was skipping P2P tests (via some strange mechanic that I didn't yet understand). Can you please rework the comment a bit?
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 updated the docs and broke out the rest into a more dedicated function. Hopefully its more clear now.
tests/integration/events.go
Outdated
|
||
case <-time.After(eventTimeout): | ||
require.Fail(s.t, "timeout waiting for update event") | ||
expect := make(map[string]struct{}) |
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.
todo: Please set the length in the make
call instead of resizing the map for each item.
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.
Done. I didn't know you could pre allocate maps.
tests/integration/events.go
Outdated
// getEventsForDelete returns a map of expected update events for a document delete action. | ||
func getEventsForDelete(docID client.DocID) map[string]struct{} { | ||
expect := make(map[string]struct{}) | ||
expect[docID.String()] = struct{}{} |
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.
todo: Please dont grow the map like this, please either set the length on make
, or (IMO) better replace this with:
return map[string]struct{}{
docID.String(): {},
}
I would probably delete the getEventsForDelete
entirely and just do this inline where getEventsForDelete
is called, it saves readers from having to bounce around the file when reading (I won't push for that as a todo though :)).
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 dropped the function and created the map like you suggested.
Relevant issue(s)
Resolves #2849
Description
This PR fixes an issue with primary document patch updates. We also now assert that the document updates match the expected updates. We can most likely remove the
events
test package now that every test asserts the expected events.Tasks
How has this been tested?
make test
Specify the platform(s) on which this was tested: