Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Unit test #215

Merged
merged 6 commits into from
Sep 13, 2019
Merged

Unit test #215

merged 6 commits into from
Sep 13, 2019

Conversation

rghetia
Copy link
Contributor

@rghetia rghetia commented Sep 13, 2019

This contains restructured tests using API only. The test is in stackdriver_test package.
This is not complete yet but framework is there add more test cases. I'll add more cases but would like to get a review on what is being done so far.

What is remaining?

  • resources
  • built-in metrics
  • metric prefix
  • name truncation

@codecov-io
Copy link

codecov-io commented Sep 13, 2019

Codecov Report

Merging #215 into master will increase coverage by 1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #215   +/-   ##
=======================================
+ Coverage   71.62%   72.63%   +1%     
=======================================
  Files          15       15           
  Lines        1586     1586           
=======================================
+ Hits         1136     1152   +16     
+ Misses        370      355   -15     
+ Partials       80       79    -1
Impacted Files Coverage Δ
stackdriver.go 32.98% <0%> (+2.06%) ⬆️
metrics_proto.go 88.32% <0%> (+2.39%) ⬆️
metrics_batcher.go 68.57% <0%> (+17.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 470fd47...c9b0d3c. Read the comment docs.

metrics_proto_api_test.go Outdated Show resolved Hide resolved
metrics_proto_api_test.go Outdated Show resolved Hide resolved
},
{
MetricDescriptor: inMDSummary,
Timeseries: []*metricspb.TimeSeries{
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding tests on one metric with more than one time series. That's how we found #214.

Copy link
Contributor Author

@rghetia rghetia Sep 13, 2019

Choose a reason for hiding this comment

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

#214 is caught by the summary test. Summary test here translates into 4 guage timeseries (one for each percentile). In fact, the summary test failed until I rebased to include the fix #214.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I take it back. #214 requires additional test as you suggested. What I had encountered was #209. I'll add the test in another PR.

@@ -0,0 +1,1138 @@
// Copyright 2019 , OpenCensus Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Space seems off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

return diff, i
}

func requireMetricDescriptorRequestEqual(t *testing.T, got, want []*monitoringpb.CreateMetricDescriptorRequest) (string, int) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same.

return new(empty.Empty), nil
}

func requireTimeSeriesRequestEqual(t *testing.T, got, want []*monitoringpb.CreateTimeSeriesRequest) (string, int) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same.

}
}

type fakeMetricsServer struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot reuse that because test is part of stackdriver_test package. However, I plan to consolidate test utility in internal package and then reuse it. But that would be another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sense to me.

}

var gotTimeSeries []*monitoringpb.CreateTimeSeriesRequest
server.forEachStackdriverTimeSeries(func(sdt *monitoringpb.CreateTimeSeriesRequest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

May be simpler to just do gotTimeSeries := server.stackdriverTimeSeries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

t.Errorf("Name[%s], TimeSeries[%d], Error: -got +want %s\n", tc.name, idx, diff)
}

var gotCreateMDRequest []*monitoringpb.CreateMetricDescriptorRequest
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

func createConn(t *testing.T, addr string) *grpc.ClientConn {
conn, err := grpc.Dial(addr, grpc.WithInsecure())
if err != nil {
t.Fatalf("Failed to make a gRPC connection to the agent: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/agent/server/g

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@bogdandrutu
Copy link
Contributor

Please add a test where:

  • no resource is set -> result global resource
  • resource is set to a different value than global -> check if project_id label is correctly set.

@rghetia
Copy link
Contributor Author

rghetia commented Sep 13, 2019

Please add a test where:

  • no resource is set -> result global resource
  • resource is set to a different value than global -> check if project_id label is correctly set.

Is another PR ok? I am working on other tests as listed in this comment

@bogdandrutu
Copy link
Contributor

Also probably in a different PR consider to use input file for the input/output to avoid a lot of the code

@rghetia rghetia merged commit a3453a5 into census-ecosystem:master Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants