Skip to content
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

cleanup: switching to stubserver in tests instead of testservice implementation #7708

Merged
merged 17 commits into from
Oct 28, 2024

Conversation

janardhanvissa
Copy link
Contributor

@janardhanvissa janardhanvissa commented Oct 8, 2024

Partially Addresses: #7291

RELEASE NOTES: None

@janardhanvissa janardhanvissa changed the title Removing testservice impl and using stubserver support for RPC calls stubserver: using stubserver support for RPC calls instead of testservice implementation Oct 8, 2024
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.26%. Comparing base (bdd444d) to head (dbcdf4d).
Report is 33 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7708      +/-   ##
==========================================
- Coverage   81.82%   81.26%   -0.57%     
==========================================
  Files         361      368       +7     
  Lines       27828    36799    +8971     
==========================================
+ Hits        22771    29904    +7133     
- Misses       3857     5652    +1795     
- Partials     1200     1243      +43     

see 322 files with indirect coverage changes

@janardhanvissa janardhanvissa changed the title stubserver: using stubserver support for RPC calls instead of testservice implementation stubserver: using stubserver for RPC calls instead of testservice implementation Oct 8, 2024
@purnesh42H purnesh42H changed the title stubserver: using stubserver for RPC calls instead of testservice implementation cleanup: switching to stubserver in tests instead of testservice implementation Oct 9, 2024
@purnesh42H
Copy link
Contributor

Can you mention the tests in description you are modifying?

@purnesh42H
Copy link
Contributor

@janardhanvissa can you split the PRs? 1) orca/, 2) test/xds, 3) rest of xds/

// will trigger the injection of custom backend metrics from the
// testServiceImpl.
// StubServer.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: stubServer

const numRequests = 20
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

why change the timeout?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, defaultTestTimeout is usually 10 secs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is 5 secs, getting context deadline issues. So, updated to 10 secs.

@@ -138,24 +136,25 @@ func (s) TestE2E_CustomBackendMetrics_OutOfBand(t *testing.T) {
t.Fatalf("Failed to create a stream for out-of-band metrics")
}

// Wait for the goroutine to finish before processing metrics.
if err := <-errCh; err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this moved out of for loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is passing after only a couple of counts, that the metrics might be sent before all the unary calls have been processed. I'm suspecting that the metrics are being flushed too early or there's a synchronization issue with the reporting of metrics. Instead of immediately checking for the expected metrics after sending the requests, we should wait for the goroutine that sends the requests to complete before processing the metrics.

default:
}

wantProto := &v3orcapb.OrcaLoadReport{
CpuUtilization: 50.0,
MemUtilization: 0.9,
ApplicationUtilization: 1.2,
Utilization: map[string]float64{requestsMetricKey: numRequests * 0.01},
Utilization: map[string]float64{requestsMetricKey: float64(numRequests) * 0.01},
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change related? Let's avoid unrelated changes

@purnesh42H purnesh42H added this to the 1.69 Release milestone Oct 16, 2024
@janardhanvissa janardhanvissa force-pushed the stub-server-support branch 2 times, most recently from 1cc0f68 to 8e937ad Compare October 16, 2024 05:49
}
},
}
// Initialize an xDS-enabled gRPC server and use the helper to start the test service.
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment here. Just modify the existing statement instead of writing a new line

test/xds/xds_server_serving_mode_test.go Show resolved Hide resolved
@@ -62,13 +63,21 @@ func (s) TestServerSideXDS_RedundantUpdateSuppression(t *testing.T) {
updateCh <- args.Mode
})

// Initialize an xDS-enabled gRPC server and register the stubServer on it.
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@@ -206,13 +214,20 @@ func (s) TestServerSideXDS_ServingModeChanges(t *testing.T) {
}
})

// Initialize an xDS-enabled gRPC server and register the stubServer on it.
// Initialize an xDS-enabled gRPC server and use the helper to start the test service.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Initialize an xDS-enabled gRPC server and use the helper to start the test service/Initialize an xDS-enabled gRPC server and sets it on stub server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comments and pushed the changes.

Copy link
Contributor

@purnesh42H purnesh42H left a comment

Choose a reason for hiding this comment

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

lgtm

@purnesh42H
Copy link
Contributor

You need to rebase to latest main branch to unblock from vet check failure

@purnesh42H
Copy link
Contributor

adding @easwars for second review

// credentials, and register the test service on it. Configure a mode change
// option that closes a channel when listener2 enter serving mode.
// Create an xDS-enabled gRPC server that is configured to use xDS
// credentials and sets it on a stub server, configuring a mode change
Copy link
Contributor

Choose a reason for hiding this comment

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

s/sets it on a stub server/assigned to a stub server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed the comments and pushed the changes. Please re-review once.

@easwars easwars assigned purnesh42H and unassigned easwars Oct 24, 2024
@purnesh42H purnesh42H merged commit ada6787 into grpc:master Oct 28, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants