Add tests for cmd/jaeger/internal/extension/jaegerstorage #5096

chirag-ghosh · 2024-01-12T14:57:33Z

Which problem is this PR solving?

Implement missing unit tests #5068

Description of the changes

Add unit tests for jaegerstorage

How was this change tested?

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

Signed-off-by: Chirag Ghosh <[email protected]>

github-actions · 2024-01-12T15:03:27Z

Test Results

2 071 tests +6 2 061 ✅ +6 1m 8s ⏱️ -1s
216 suites ±0 10 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit fb1a3b4. ± Comparison against base commit 9997cce.

♻️ This comment has been updated with latest results.

codecov · 2024-01-12T15:06:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9997cce) 95.56% compared to head (fb1a3b4) 95.56%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5096   +/-   ##
=======================================
  Coverage   95.56%   95.56%           
=======================================
  Files         317      320    +3     
  Lines       18283    18347   +64     
=======================================
+ Hits        17472    17534   +62     
- Misses        651      653    +2     
  Partials      160      160

Flag	Coverage Δ
cassandra-3.x	`25.58% <ø> (ø)`
cassandra-4.x	`25.58% <ø> (ø)`
elasticsearch-5.x	`19.87% <ø> (+0.01%)`	⬆️
elasticsearch-6.x	`19.85% <ø> (-0.02%)`	⬇️
elasticsearch-7.x	`20.00% <ø> (+0.01%)`	⬆️
elasticsearch-8.x	`20.08% <ø> (ø)`
grpc-badger	`19.51% <ø> (ø)`
kafka	`14.09% <ø> (ø)`
opensearch-1.x	`20.00% <ø> (ø)`
opensearch-2.x	`19.99% <ø> (ø)`
unittests	`93.24% <100.00%> (+0.01%)`	⬆️

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.

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

yurishkuro

Another suggestion: the config does not have a Validate function like the other components have, can you add it? And then you can add a test calling getDefaultConfig and checking that Validate returns an error (because I don't think empty config is valid for this extension).

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

yurishkuro · 2024-01-12T19:16:26Z

btw very nice PR

yurishkuro · 2024-01-12T19:17:30Z

Nit: be more specific in the PR title - use full package path

Signed-off-by: Chirag Ghosh <[email protected]>

…tion. add test Signed-off-by: Chirag Ghosh <[email protected]>

chirag-ghosh · 2024-01-13T06:18:51Z

The config did not have any required fields. Memory seems to be a required field atleeast till other storages are supported. I have therefore made it required. Please check.

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

Signed-off-by: Chirag Ghosh <[email protected]>

yurishkuro

Lgtm

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

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

Signed-off-by: Chirag Ghosh <[email protected]>

yurishkuro · 2024-01-14T15:47:12Z

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

+	host := componenttest.NewNopHost()
+	err := storageExtension.Start(ctx, host)
+	require.Error(t, err)
+	require.EqualError(t, err, fmt.Sprintf("duplicate memory storage name %s", memstoreName))


Where is the error in this case?

We are starting the storageExtension again. This will lead to an error. Ref

but the error message doesn't seem to be about extension starting.

If you're testing specifically the dual entry condition, it would be better to test it explicitly, not via a side-effect of starting extension twice. I assume it's possible to create dual entry in the config if using a different type of storage and then giving it the same name.

Currently only Memory type storage is supported. So I was unable to recreate this using config. Have changed the function name for now.

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

Signed-off-by: Chirag Ghosh <[email protected]>

yurishkuro

you can have a test for bad name passed to GetStorageFactory
you can have a test for error path in Shutdown by manually inserting a mock Factory into extension.factories which throws an error on Close

Signed-off-by: Chirag Ghosh <[email protected]>

chirag-ghosh · 2024-01-16T06:45:31Z

Done
extension.factories isn't supposed to be accessible by public methods. The only place it is set is here

yurishkuro · 2024-01-16T06:51:50Z

extension.factories isn't supposed to be accessible by public methods.

tests can be defined in the same package and have access to private members

Signed-off-by: Chirag Ghosh <[email protected]>

chirag-ghosh · 2024-01-16T16:30:40Z

Done. Please check

yurishkuro · 2024-01-16T19:27:03Z

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

+}
+
+func (e errorFactory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger) error {
+	panic("implement me")


nit: change to "not implemented", otherwise it reads as if these need to be implemented

Done. Maybe we should also change this then.

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

Signed-off-by: Chirag Ghosh <[email protected]>

yurishkuro

Thanks!

feat: add jaegerstorage test

6391d07

Signed-off-by: Chirag Ghosh <[email protected]>

chirag-ghosh requested a review from a team as a code owner January 12, 2024 14:57

chirag-ghosh requested a review from yurishkuro January 12, 2024 14:57

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

yurishkuro reviewed Jan 12, 2024

View reviewed changes

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

yurishkuro reviewed Jan 12, 2024

View reviewed changes

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

yurishkuro changed the title ~~Add tests for jaegerstorage~~ Add tests for cmd/jaeger/internal/extension/jaegerstorage Jan 12, 2024

chirag-ghosh added 2 commits January 13, 2024 11:34

feat: move cleanup shutdown function to makeStorageExtension

4df37af

Signed-off-by: Chirag Ghosh <[email protected]>

feat: make Memory in jaegerstorage config required. add validate func…

86021b0

…tion. add test Signed-off-by: Chirag Ghosh <[email protected]>

yurishkuro reviewed Jan 13, 2024

View reviewed changes

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

fix: change config validation

67f5051

Signed-off-by: Chirag Ghosh <[email protected]>

yurishkuro reviewed Jan 14, 2024

View reviewed changes

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

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

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

fix: pr suggestion changes

14e94fc

Signed-off-by: Chirag Ghosh <[email protected]>

yurishkuro reviewed Jan 14, 2024

View reviewed changes

yurishkuro reviewed Jan 15, 2024

View reviewed changes

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

fix: pr suggestion changes

9860703

Signed-off-by: Chirag Ghosh <[email protected]>

yurishkuro requested changes Jan 15, 2024

View reviewed changes

feat: add bad name GetStorageFactory test

ba654aa

Signed-off-by: Chirag Ghosh <[email protected]>

feat: add bad factory shutdown error

09f1c20

Signed-off-by: Chirag Ghosh <[email protected]>

yurishkuro reviewed Jan 16, 2024

View reviewed changes

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

yurishkuro reviewed Jan 16, 2024

View reviewed changes

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

yurishkuro reviewed Jan 16, 2024

View reviewed changes

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

chirag-ghosh and others added 2 commits January 17, 2024 09:45

fix: pr suggestion changes

2a74ec4

Signed-off-by: Chirag Ghosh <[email protected]>

Merge branch 'main' into storageextension-test

fb1a3b4

yurishkuro approved these changes Jan 18, 2024

View reviewed changes

yurishkuro enabled auto-merge (squash) January 18, 2024 05:02

yurishkuro merged commit 1c0a6ca into jaegertracing:main Jan 18, 2024
39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for cmd/jaeger/internal/extension/jaegerstorage #5096

Add tests for cmd/jaeger/internal/extension/jaegerstorage #5096

chirag-ghosh commented Jan 12, 2024

github-actions bot commented Jan 12, 2024 •

edited

Loading

codecov bot commented Jan 12, 2024 •

edited

Loading

yurishkuro left a comment

yurishkuro commented Jan 12, 2024 •

edited

Loading

yurishkuro commented Jan 12, 2024

chirag-ghosh commented Jan 13, 2024

yurishkuro left a comment

yurishkuro Jan 14, 2024

chirag-ghosh Jan 15, 2024

yurishkuro Jan 15, 2024 •

edited

Loading

yurishkuro Jan 15, 2024

chirag-ghosh Jan 15, 2024 •

edited

Loading

yurishkuro left a comment

chirag-ghosh commented Jan 16, 2024 •

edited

Loading

yurishkuro commented Jan 16, 2024

chirag-ghosh commented Jan 16, 2024

yurishkuro Jan 16, 2024

chirag-ghosh Jan 17, 2024

yurishkuro left a comment

Add tests for cmd/jaeger/internal/extension/jaegerstorage #5096

Add tests for cmd/jaeger/internal/extension/jaegerstorage #5096

Conversation

chirag-ghosh commented Jan 12, 2024

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

github-actions bot commented Jan 12, 2024 • edited Loading

Test Results

codecov bot commented Jan 12, 2024 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro commented Jan 12, 2024 • edited Loading

yurishkuro commented Jan 12, 2024

chirag-ghosh commented Jan 13, 2024

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro Jan 14, 2024

Choose a reason for hiding this comment

chirag-ghosh Jan 15, 2024

Choose a reason for hiding this comment

yurishkuro Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

yurishkuro Jan 15, 2024

Choose a reason for hiding this comment

chirag-ghosh Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

chirag-ghosh commented Jan 16, 2024 • edited Loading

yurishkuro commented Jan 16, 2024

chirag-ghosh commented Jan 16, 2024

yurishkuro Jan 16, 2024

Choose a reason for hiding this comment

chirag-ghosh Jan 17, 2024

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 12, 2024 •

edited

Loading

codecov bot commented Jan 12, 2024 •

edited

Loading

yurishkuro commented Jan 12, 2024 •

edited

Loading

yurishkuro Jan 15, 2024 •

edited

Loading

chirag-ghosh Jan 15, 2024 •

edited

Loading

chirag-ghosh commented Jan 16, 2024 •

edited

Loading