Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Improve Test coverage #251

Merged

Conversation

yu-sun-77
Copy link
Contributor

@yu-sun-77 yu-sun-77 commented Dec 1, 2020

Fixes #, if available:

Description of changes:
Improve Test coverage from 19% to 40%

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Dec 1, 2020

Codecov Report

Merging #251 (d1e70a6) into master (4713ae2) will increase coverage by 12.30%.
The diff coverage is 0.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master     #251       +/-   ##
=============================================
+ Coverage     19.47%   31.77%   +12.30%     
- Complexity       80      116       +36     
=============================================
  Files            39       39               
  Lines          2008     2008               
  Branches        150      150               
=============================================
+ Hits            391      638      +247     
+ Misses         1598     1335      -263     
- Partials         19       35       +16     
Impacted Files Coverage Δ Complexity Δ
...alyzer/collectors/CacheConfigMetricsCollector.java 77.14% <ø> (+77.14%) 6.00 <0.00> (+6.00)
...ormanceanalyzer/writer/EventLogQueueProcessor.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...elasticsearch/performanceanalyzer/ESResources.java 52.63% <0.00%> (+7.89%) 8.00% <0.00%> (+2.00%)
...llectors/NodeStatsFixedShardsMetricsCollector.java 75.51% <0.00%> (+75.51%) 10.00% <0.00%> (+10.00%)
...manceanalyzer/collectors/NodeDetailsCollector.java 77.96% <0.00%> (+77.96%) 8.00% <0.00%> (+8.00%)
...nalyzer/collectors/ThreadPoolMetricsCollector.java 89.55% <0.00%> (+89.55%) 10.00% <0.00%> (+10.00%)

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 2ca08c7...7eea5eb. Read the comment docs.

@yu-sun-77 yu-sun-77 requested a review from ktkrg December 2, 2020 18:31
Copy link
Contributor

@ktkrg ktkrg left a comment

Choose a reason for hiding this comment

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

Just one comment around a missing negative test scenario, looks good otherwise!

@yu-sun-77 yu-sun-77 force-pushed the test-coverage branch 3 times, most recently from ed2438c to 7eea5eb Compare December 4, 2020 20:27
Copy link
Contributor

@adityaj1107 adityaj1107 left a comment

Choose a reason for hiding this comment

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

I have minor non blocking comments. Approving this version, since those can be picked up in subsequent versions as well.

threadPool.shutdownNow();
super.tearDown();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add saveMetricsValues test here as well?

@Test
public void testCollectMetrics() {
createIndex(TEST_INDEX);
Mockito.when(controller.getNodeStatsShardsPerCollection()).thenReturn(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a test to check if all the metrics for all the shards are populated in subsequent runs of this collector?

public void tearDown() throws Exception {
super.tearDown();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add saveMetricsValues test here as well?

CacheMaxSizeStatus filedDataCache = metrics.get(0);
CacheMaxSizeStatus shardRequestCache = metrics.get(1);
assertEquals(CacheType.FIELD_DATA_CACHE.toString(), filedDataCache.getCacheType());
assertEquals(CacheType.SHARD_REQUEST_CACHE.toString(), shardRequestCache.getCacheType());
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we assert values for Cache Sizes as well?

@ktkrg ktkrg self-requested a review December 5, 2020 04:15
Copy link
Contributor

@ktkrg ktkrg left a comment

Choose a reason for hiding this comment

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

Just noticed that some files are missing copyright headers. Please create an issue to add them before merging this PR.

@@ -0,0 +1,66 @@
package com.amazon.opendistro.elasticsearch.performanceanalyzer.collectors;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Missing license header.

@@ -0,0 +1,80 @@
package com.amazon.opendistro.elasticsearch.performanceanalyzer.collectors;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing license header

@yu-sun-77
Copy link
Contributor Author

created #253 to track missing license headers for new files.

@yu-sun-77 yu-sun-77 merged commit b2ed391 into opendistro-for-elasticsearch:master Dec 7, 2020
@yu-sun-77 yu-sun-77 deleted the test-coverage branch December 7, 2020 20:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants