Add tests for internal/extension/jaegerquery #5123

VaibhavMalik4187 · 2024-01-19T19:20:15Z

Which problem is this PR solving?

Part of Implement missing unit tests #5068

Description of the changes

This commit adds tests for the cmd/jaeger/internal/extension/jaegerquery package.

How was this change tested?

make test

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

yurishkuro

please provide output of

go test -cover ./cmd/jaeger/internal/extension/jaegerquery/

cmd/jaeger/internal/extension/jaegerquery/config_test.go

cmd/jaeger/internal/extension/jaegerquery/factory_test.go

cmd/jaeger/internal/extension/jaegerquery/server_test.go

VaibhavMalik4187 · 2024-01-21T08:33:10Z

please provide output of

go test -cover ./cmd/jaeger/internal/extension/jaegerquery/

ok github.com/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerquery 0.013s coverage: 84.1% of statements

codecov · 2024-01-21T18:09:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.30%. Comparing base (12aa9da) to head (0a46523).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5123      +/-   ##
==========================================
+ Coverage   94.91%   95.30%   +0.39%     
==========================================
  Files         334      334              
  Lines       16230    16233       +3     
==========================================
+ Hits        15404    15471      +67     
+ Misses        648      582      -66     
- Partials      178      180       +2

Flag	Coverage Δ
badger_v1	`7.97% <ø> (ø)`
badger_v2	`1.88% <0.00%> (ø)`
cassandra-3.x-v1	`16.27% <ø> (ø)`
cassandra-3.x-v2	`1.81% <0.00%> (ø)`
cassandra-4.x-v1	`16.27% <ø> (ø)`
cassandra-4.x-v2	`1.81% <0.00%> (ø)`
elasticsearch-5.x	`1.73% <0.00%> (?)`
elasticsearch-6.x	`1.73% <0.00%> (?)`
grpc_v1	`10.13% <ø> (ø)`
grpc_v2	`7.36% <0.00%> (ø)`
kafka	`9.68% <ø> (ø)`
opensearch-1.x	`1.73% <0.00%> (+0.01%)`	⬆️
opensearch-2.x	`1.72% <0.00%> (-0.02%)`	⬇️
unittests	`93.51% <100.00%> (+0.39%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yurishkuro

Need to fix linter errors (run make lint)

go test -cover ./cmd/jaeger/internal/extension/jaegerquery/
ok github.com/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerquery 0.013s coverage: 84.1% of statements

needs to be above 95%

cmd/jaeger/internal/extension/jaegerquery/factory_test.go

cmd/jaeger/internal/extension/jaegerquery/server_test.go

VaibhavMalik4187 · 2024-01-23T11:20:02Z

Need to fix linter errors (run make lint)

go test -cover ./cmd/jaeger/internal/extension/jaegerquery/
ok github.com/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerquery 0.013s coverage: 84.1% of statements

needs to be above 95%

I'm trying but the uncovered statements are kind of difficult to reach. For example:

Partially Addresses: jaegertracing#5068 - This commit adds tests for the `cmd/jaeger/internal/extension/jaegerquery` package. It brings the code coverage of the package to 84% - make test - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: VaibhavMalik4187 <[email protected]>

cmd/jaeger/internal/extension/jaegerquery/server_test.go

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro · 2024-01-23T23:25:07Z

I added example of another test: 7af4dc1

coverage: 86.4% of statements

yurishkuro · 2024-01-23T23:34:43Z

This is what I would suggest:

in order to simulate the errors you really need to be able to provide a fake storage factory
the factories are retrieved via jaegerstorage.GetStorageFactory util function, which internally looks up the extension in the host and tries to cast it to private type to access the factories map f, ok := comp.(*storageExt).factories[name]
to allow this to be mocked, we can change this to f, ok := comp.(StorageExt).Factory(name) with a new interface:

type StorageExt interface {
  Factory(string) (storage.Factory, error)
}

and have storageExt implement this interface. Then in the test you can create a fake implementation of StorageExt that returns a fake factory, and store that fake impl in your fake hostExtension

yurishkuro · 2024-02-09T17:59:22Z

@VaibhavMalik4187 do you plan to finish this?

VaibhavMalik4187 · 2024-02-09T18:10:13Z

@VaibhavMalik4187 do you plan to finish this?

I'm sorry for being inactive. I'll finish this soon. Thanks for bearing with me.

yurishkuro · 2024-02-09T18:35:11Z

No worries, I just don't want this to rot & get lost, as these are valuable changes.

VaibhavMalik4187 · 2024-02-09T21:20:41Z

No worries, I just don't want this to rot & get lost, as these are valuable changes.

Quick update, I've tried to incorporate your suggestions and now the code coverage of this package has crossed the 95% mark. Could you please go through this PR once again? Many thanks!

This commit includes the implementation of missing tests for jaegerquery server defined in the `internal/extension/jaegerquery` package. With the addition of these tests, the code coverage of this package bumps to >95% This commit also incorporates a small change in the jaegerstorage extension. The `GetStorageFactory` method will now accept `StorageExt` interface from the host. This allowed more comprehensive testing using mock objects. Signed-off-by: VaibhavMalik4187 <[email protected]>

cmd/jaeger/internal/extension/jaegerstorage/extension.go

yurishkuro · 2024-02-09T22:38:31Z

cmd/jaeger/internal/extension/jaegerquery/server_test.go

+}
+
+func (ff fakeFactory) CreateDependencyReader() (dependencystore.Reader, error) {
+	if ff.name == "need-dependency-reader-error" {


this is not a great pattern, I suggest you simply store the Error object in fakeFactory that it's expected to return

cmd/jaeger/internal/extension/jaegerquery/server_test.go

cmd/jaeger/internal/extension/jaegerstorage/extension.go

yurishkuro

please address all open comments

cmd/jaeger/internal/extension/jaegerquery/server_test.go

VaibhavMalik4187 · 2024-02-12T04:24:52Z

please address all open comments

On it.

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro · 2024-05-08T23:22:04Z

Thanks, @VaibhavMalik4187

VaibhavMalik4187 requested a review from a team as a code owner January 19, 2024 19:20

VaibhavMalik4187 requested a review from jkowall January 19, 2024 19:20

VaibhavMalik4187 force-pushed the internal-extension-jaegerquery-tests branch from cd84a57 to 033ea71 Compare January 19, 2024 19:21

yurishkuro reviewed Jan 19, 2024

View reviewed changes

VaibhavMalik4187 force-pushed the internal-extension-jaegerquery-tests branch from 033ea71 to bb28873 Compare January 21, 2024 08:27

VaibhavMalik4187 force-pushed the internal-extension-jaegerquery-tests branch from bb28873 to 7918d8a Compare January 21, 2024 18:00

yurishkuro added the changelog:test Change that's adding missing tests or correcting existing tests label Jan 21, 2024

yurishkuro changed the title ~~Added tests for internal/extension/jaegerquery~~ Add tests for internal/extension/jaegerquery Jan 21, 2024

yurishkuro requested changes Jan 21, 2024

View reviewed changes

VaibhavMalik4187 force-pushed the internal-extension-jaegerquery-tests branch 2 times, most recently from e81e4fe to f9fe693 Compare January 23, 2024 11:15

VaibhavMalik4187 force-pushed the internal-extension-jaegerquery-tests branch from f9fe693 to 0362bfd Compare January 23, 2024 11:22

yurishkuro reviewed Jan 23, 2024

View reviewed changes

cmd/jaeger/internal/extension/jaegerquery/server_test.go Outdated Show resolved Hide resolved

yurishkuro reviewed Jan 23, 2024

View reviewed changes

cmd/jaeger/internal/extension/jaegerquery/server_test.go Outdated Show resolved Hide resolved

yurishkuro reviewed Jan 23, 2024

View reviewed changes

cmd/jaeger/internal/extension/jaegerquery/server_test.go Outdated Show resolved Hide resolved

Add test

7af4dc1

Signed-off-by: Yuri Shkuro <[email protected]>

Merge branch 'main' into internal-extension-jaegerquery-tests

0446c19

VaibhavMalik4187 force-pushed the internal-extension-jaegerquery-tests branch from 13db5bf to efb09b8 Compare February 10, 2024 10:37

yurishkuro reviewed Feb 10, 2024

View reviewed changes

VaibhavMalik4187 commented Feb 11, 2024

View reviewed changes

cmd/jaeger/internal/extension/jaegerstorage/extension.go Outdated Show resolved Hide resolved

yurishkuro reviewed Feb 11, 2024

View reviewed changes

james-ryans mentioned this pull request Mar 2, 2024

[jaeger-v2] Add support for artificial jaeger storage receiver #5242

Merged

4 tasks

yurishkuro and others added 5 commits May 8, 2024 15:52

Merge branch 'main' into internal-extension-jaegerquery-tests

d68785b

Signed-off-by: Yuri Shkuro <[email protected]>

fix

bcae45b

Signed-off-by: Yuri Shkuro <[email protected]>

check leaks

51e23f0

Signed-off-by: Yuri Shkuro <[email protected]>

fixes

4a0db47

Signed-off-by: Yuri Shkuro <[email protected]>

remove unnecessary test

0a46523

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro approved these changes May 8, 2024

View reviewed changes

yurishkuro enabled auto-merge (squash) May 8, 2024 23:04

yurishkuro merged commit 5ddcaa9 into jaegertracing:main May 8, 2024
40 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for internal/extension/jaegerquery #5123

Add tests for internal/extension/jaegerquery #5123

VaibhavMalik4187 commented Jan 19, 2024 •

edited by yurishkuro

Loading

yurishkuro left a comment

VaibhavMalik4187 commented Jan 21, 2024

codecov bot commented Jan 21, 2024 •

edited

Loading

yurishkuro left a comment

VaibhavMalik4187 commented Jan 23, 2024

yurishkuro commented Jan 23, 2024

yurishkuro commented Jan 23, 2024

yurishkuro commented Feb 9, 2024

VaibhavMalik4187 commented Feb 9, 2024

yurishkuro commented Feb 9, 2024

VaibhavMalik4187 commented Feb 9, 2024

yurishkuro Feb 9, 2024

yurishkuro left a comment

VaibhavMalik4187 commented Feb 12, 2024

yurishkuro commented May 8, 2024

Add tests for internal/extension/jaegerquery #5123

Add tests for internal/extension/jaegerquery #5123

Conversation

VaibhavMalik4187 commented Jan 19, 2024 • edited by yurishkuro Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

yurishkuro left a comment

Choose a reason for hiding this comment

VaibhavMalik4187 commented Jan 21, 2024

codecov bot commented Jan 21, 2024 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

VaibhavMalik4187 commented Jan 23, 2024

yurishkuro commented Jan 23, 2024

yurishkuro commented Jan 23, 2024

yurishkuro commented Feb 9, 2024

VaibhavMalik4187 commented Feb 9, 2024

yurishkuro commented Feb 9, 2024

VaibhavMalik4187 commented Feb 9, 2024

yurishkuro Feb 9, 2024

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

VaibhavMalik4187 commented Feb 12, 2024

yurishkuro commented May 8, 2024

VaibhavMalik4187 commented Jan 19, 2024 •

edited by yurishkuro

Loading

codecov bot commented Jan 21, 2024 •

edited

Loading