Add integration test for Resync request subscriber #488
Conversation
Summary of ChangesHello @asmit990, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a crucial integration test designed to validate the MeshSync resynchronization process. By simulating a full resync flow, the test aims to ensure that MeshSync correctly republishes resource events after receiving a resync request. The current failure of this test highlights a previously identified bug, providing concrete evidence and a reproducible scenario for the missing functionality. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new integration test for the MeshSync resync flow. The test is well-structured and correctly identifies a behavioral issue as described. My review includes several suggestions to enhance the test's maintainability and robustness by addressing hardcoded values, an unhandled error, and code duplication. These changes will make the test more consistent with the existing test suite and easier to manage in the future.
| Entity: broker.ReSyncDiscoveryEntity, | ||
| }, | ||
| } | ||
| payload, _ := json.Marshal(req) |
There was a problem hiding this comment.
The error returned by json.Marshal is ignored. While it's unlikely to fail for this struct, it's a best practice to always handle errors. An error could occur if the struct contains types that cannot be marshaled.
| payload, _ := json.Marshal(req) | |
| payload, err := json.Marshal(req) | |
| if err != nil { | |
| t.Fatalf("failed to marshal resync request: %v", err) | |
| } |
| ) | ||
|
|
||
| func TestIntegrationResyncFlowViaBroker(t *testing.T) { | ||
| brokerURL := "nats://localhost:4222" |
There was a problem hiding this comment.
For consistency with other integration tests in this package, consider using the testMeshsyncNatsURL variable instead of a hardcoded string for the broker URL. This variable is defined in integration_test.go and is accessible within the same package.
| brokerURL := "nats://localhost:4222" | |
| brokerURL := testMeshsyncNatsURL |
| _ = cfg | ||
|
|
||
| // subjects | ||
| requestSubject := "meshery.meshsync.request" |
There was a problem hiding this comment.
The request subject is hardcoded. To improve maintainability and avoid magic strings, it's better to use the value defined in the configuration. You can get it from config.Listeners[config.RequestStream].SubscribeTo.
| requestSubject := "meshery.meshsync.request" | |
| requestSubject := config.Listeners[config.RequestStream].SubscribeTo |
| defer sub.Unsubscribe() | ||
|
|
||
| // STEP 1 — wait for first discovery event (MeshSync must warm up) | ||
| warmupTimeout := time.After(120 * time.Second) |
There was a problem hiding this comment.
The timeout duration of 120 seconds is hardcoded. This value is also used on line 72. Consider defining this as a constant to improve readability and make it easier to change in one place.
For example:
const testTimeout = 120 * time.Second| case msg := <-msgs: | ||
| var m broker.Message | ||
| if json.Unmarshal(msg.Data, &m) == nil && m.Object != nil { | ||
| receivedInitial = true | ||
| } |
There was a problem hiding this comment.
The logic for unmarshaling and validating a NATS message is duplicated here and again at lines 75-80. To improve code clarity and reduce duplication, consider extracting this logic into a helper function.
For example:
func isResourceEvent(msg *nats.Msg) bool {
var m broker.Message
return json.Unmarshal(msg.Data, &m) == nil && m.Object != nil
}You could then call this function in your select cases: if isResourceEvent(msg) { ... }.
n2h9
left a comment
n2h9
left a comment
There was a problem hiding this comment.
Hello @asmit990 👋 !
Thank you for your contribution 👍 .
There are couple checks failing, which needs to be addressed on the first place.
- DCO check (commit signing);
- lint;
- integration test itself is failing;
Currently the test fails because MeshSync does not emit resource events after a resync.
This confirms the missing behavior referenced in the issue.
I have doubts about this, we tested it manually multiple times in the past couple months and resync functionality was working properly, f.e. during fixing this on meshery side: meshery/meshery#15671
I think nothing changed since that time in meshsync.
Also if something is not working, it is a good opportunity to fix it.
2 participants
Adds integration coverage for Resync flow:
Currently the test fails because MeshSync does not emit resource events after a resync.
This confirms the missing behavior referenced in the issue.