-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[chore][tests] Clean up integration tests to remove archive reader / writer #6625
[chore][tests] Clean up integration tests to remove archive reader / writer #6625
Conversation
Signed-off-by: Mahad Zaryab <[email protected]>
af := s.initializeCassandraFactory(t, []string{ | ||
"--cassandra-archive.keyspace=jaeger_v1_dc1_archive", | ||
"--cassandra-archive.enabled=true", | ||
"--cassandra-archive.servers=127.0.0.1", | ||
"--cassandra-archive.basic.allowed-authenticators=org.apache.cassandra.auth.PasswordAuthenticator", | ||
"--cassandra-archive.password=" + password, | ||
"--cassandra-archive.username=" + username, | ||
}, cassandra.NewArchiveFactory) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro thoughts on keeping vs removing this? we were initializing the archive span reader / writer but never actual ran the archive test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6625 +/- ##
==========================================
- Coverage 95.97% 95.92% -0.06%
==========================================
Files 365 365
Lines 20616 20616
==========================================
- Hits 19787 19776 -11
- Misses 630 640 +10
- Partials 199 200 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
tID := model.NewTraceID(uint64(11), uint64(22)) | ||
expected := &model.Span{ | ||
OperationName: "archive_span", | ||
StartTime: time.Now().Add(-maxSpanAge * 5).Truncate(time.Microsecond), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maxSpanAge is a constant in this file, but it is not passed as a configuration to the storage factory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro its just using the default - https://github.com/jaegertracing/jaeger/blob/main/plugin/storage/es/options.go#L403
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, but it's disassociated from that default. We should pass max age as parameter to the factory here.
Which problem is this PR solving?
Description of the changes
elasticsearch_test.go
.How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:npm run lint
andnpm run test