Skip to content

Add integration test for Resync request subscriber #488

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.

Sign up for GitHub

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

Open
asmit990 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add integration test for Resync request subscriber #488

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions integration-tests/meshsync_resync_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package tests

import (
"encoding/json"
"testing"
"time"

"github.com/meshery/meshkit/broker"
"github.com/nats-io/nats.go"

Check failure on line 9 in integration-tests/meshsync_resync_integration_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint (ubuntu-24.04, 1.24.x) 

File is not properly formatted (gci)

"github.com/meshery/meshsync/internal/config"
)

func TestIntegrationResyncFlowViaBroker(t *testing.T) {

Check failure on line 14 in integration-tests/meshsync_resync_integration_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint (ubuntu-24.04, 1.24.x) 

calculated cyclomatic complexity for function TestIntegrationResyncFlowViaBroker is 15, max is 10 (cyclop)
brokerURL := "nats://localhost:4222"
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
brokerURL := "nats://localhost:4222"
brokerURL := testMeshsyncNatsURL


// connect to broker
nc, err := nats.Connect(brokerURL)
if err != nil {
t.Fatalf("failed to connect to broker: %v", err)
}
defer nc.Drain()

Check failure on line 22 in integration-tests/meshsync_resync_integration_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint (ubuntu-24.04, 1.24.x) 

Error return value of `nc.Drain` is not checked (errcheck)

// load config
cfg, err := config.New("viper")
if err != nil {
t.Fatalf("failed to load config: %v", err)
}

// listener config (loaded even if unused)
_ = cfg

// subjects
requestSubject := "meshery.meshsync.request"
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
requestSubject := "meshery.meshsync.request"
requestSubject := config.Listeners[config.RequestStream].SubscribeTo

responseSubject := config.DefaultPublishingSubject

// subscribe to resource events
msgs := make(chan *nats.Msg, 200)
sub, err := nc.ChanSubscribe(responseSubject, msgs)
if err != nil {
t.Fatalf("failed to subscribe to resource subject: %v", err)
}
defer sub.Unsubscribe()

Check failure on line 43 in integration-tests/meshsync_resync_integration_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint (ubuntu-24.04, 1.24.x) 

Error return value of `sub.Unsubscribe` is not checked (errcheck)

// STEP 1 — wait for first discovery event (MeshSync must warm up)
warmupTimeout := time.After(120 * time.Second)
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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

receivedInitial := false
for !receivedInitial {
select {
case msg := <-msgs:
var m broker.Message
if json.Unmarshal(msg.Data, &m) == nil && m.Object != nil {
receivedInitial = true
}
Comment on lines +50 to +54
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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) { ... }.

case <-warmupTimeout:
t.Fatalf("timeout: initial discovery events never arrived — MeshSync never warmed up")
}
}

// STEP 2 — send resync AFTER MeshSync is initialized
req := &broker.Message{
Request: &broker.RequestObject{
Entity: broker.ReSyncDiscoveryEntity,
},
}
payload, _ := json.Marshal(req)
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
payload, _ := json.Marshal(req)
payload, err := json.Marshal(req)
if err != nil {
t.Fatalf("failed to marshal resync request: %v", err)
}

if err := nc.Publish(requestSubject, payload); err != nil {
t.Fatalf("failed to publish resync request: %v", err)
}

// STEP 3 — expect NEW resource events after resync
afterTimeout := time.After(120 * time.Second)
for {
select {
case msg := <-msgs:
var m broker.Message
if json.Unmarshal(msg.Data, &m) == nil && m.Object != nil {
// resync confirmed
return
}
case <-afterTimeout:
t.Fatalf("timeout: no resource events were received after resync request")
}
}
}
Loading