e2e index cleaner tests improvements #1331
Conversation
Signed-off-by: Jeeva Kandasamy <jkandasa@gmail.com>
Signed-off-by: Jeeva Kandasamy <jkandasa@gmail.com>
|
@kevinearls @jpkrohling @objectiser kindly review the changes |
Codecov Report
@@ Coverage Diff @@
## master #1331 +/- ##
==========================================
- Coverage 87.41% 86.02% -1.39%
==========================================
Files 89 90 +1
Lines 5021 5125 +104
==========================================
+ Hits 4389 4409 +20
- Misses 467 548 +81
- Partials 165 168 +3
Continue to review full report at Codecov.
|
Signed-off-by: Jeeva Kandasamy <jkandasa@gmail.com>
|
@jkandasa Is this something we need to do on a per PR basis? If not it might be better to move this to its own test suite (like elasticsearch_index_tests.go) and only run it on an as needed basis. |
test/e2e/elasticsearch_test.go
Outdated
| // Run the smoke test so indices will be created | ||
| AllInOneSmokeTest(jaegerInstanceName) | ||
| // executes index cleaner tests | ||
| func (suite *ElasticSearchTestSuite) runIndexCleaner(esIndexPrefix string, daysRange []int) { |
There was a problem hiding this comment.
At 175 lines this function is really long. I don't think we have a rule, but it would be good to keep things short enough to fit on a screen. I will recommend some changes below.
test/e2e/elasticsearch_test.go
Outdated
| AllInOneSmokeTest(jaegerInstanceName) | ||
| // function to get indices | ||
| // returns in order: serviceIndices, spansIndices | ||
| getIndices := func() ([]esIndexData, []esIndexData) { |
There was a problem hiding this comment.
I would recommend defining this as func getIndices() outside of this test function.
test/e2e/elasticsearch_test.go
Outdated
| // Once we've created a span with the smoke test, enable the index cleaner | ||
| turnOnEsIndexCleaner(jaegerInstance) | ||
| // function to validate indices | ||
| assertIndex := func(indices []esIndexData, verifyDateAfter time.Time, count int) { |
There was a problem hiding this comment.
I think it would be a good idea to extract this too.
test/e2e/elasticsearch_test.go
Outdated
| // generate spans and service for last 45 days | ||
| currentDate := time.Now() | ||
| indexDateLayout := "2006-01-02" | ||
| // enable port forward for collector port |
There was a problem hiding this comment.
nit: the comment isn't necessary as the log message says the same thing.
test/e2e/elasticsearch_test.go
Outdated
| stringDate := spanDate.Format(indexDateLayout) | ||
| // get tracing client | ||
| serviceName := fmt.Sprintf("%s_%s", jaegerInstanceName, stringDate) | ||
| tracer, closer, err := getTracerClientWithCollectorEndpoint(serviceName, fmt.Sprintf("http://localhost:%d/api/traces", localPortColl)) |
There was a problem hiding this comment.
Since this is a tracing client and not a tracer, I would call it tracingClient.
test/e2e/elasticsearch_test.go
Outdated
| // Now make sure indices have been deleted | ||
| indexWithPrefixExists("jaeger-", false, esNamespace) | ||
| // trigger the index cleaner for multiple day range and verify indices | ||
| for _, verifyDays := range daysRange { |
There was a problem hiding this comment.
Finally this block (i.e. the for loop) could be extracted to its own method.
test/e2e/elasticsearch_test.go
Outdated
| localPortColl := colPorts[0].Local | ||
|
|
||
| logrus.Infof("Generating spans and services for the last %d days", indexHistoryDays) | ||
| for day := 0; day < indexHistoryDays; day++ { |
There was a problem hiding this comment.
This for loop could be put in its own function
test/e2e/elasticsearch_test.go
Outdated
| // update and trigger index cleaner job | ||
| turnOnEsIndexCleaner(jaegerInstance, verifyDays) | ||
|
|
||
| // get servies and spans indices |
There was a problem hiding this comment.
nit: get services and spans indices
test/e2e/elasticsearch_test.go
Outdated
| func (suite *ElasticSearchTestSuite) TestEsIndexCleaner() { | ||
| // update jaeger CR with index cleaner specifications | ||
| indexHistoryDays := 45 // maximum number of days to generate spans and services | ||
| numberOfDays = indexHistoryDays |
test/e2e/elasticsearch_test.go
Outdated
| updateOptions := make(map[string]interface{}) | ||
| for key, value := range options { | ||
| updateOptions[key] = value | ||
| // update es node namespace and es node url into jaeger CR for external es deployment |
There was a problem hiding this comment.
This comment doesn't really make it clear what your doing here. I think something like this might be better:
// If there is an external es deployment use it instead of creating a self prov one
Signed-off-by: Jeeva Kandasamy <jkandasa@gmail.com>
|
@kevinearls Thanks for the review comments. I am working on the required changes. |
Signed-off-by: Jeeva Kandasamy <jkandasa@gmail.com>
Signed-off-by: Jeeva Kandasamy <jkandasa@gmail.com>
|
@kevinearls can you review the changes? Looks like a delay abd1f0e fixes the e2e test failure in github ci environment |
|
@jpkrohling Will merge this, as hasn't contributed to the project codecov drop, but looks like the |
Changes summary:
numberOfDays), 45, 30, 7, 1, 0 days