Skip to content
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

[HUDI-707]Add unit test for StatsCommand #1645

Merged
merged 3 commits into from May 21, 2020

Conversation

hddong
Copy link
Contributor

@hddong hddong commented May 20, 2020

Tips

What is the purpose of the pull request

Add unit test for StatsCommand

Brief change log

  • Add unit test for StatsCommand

Verify this pull request

This pull request is a trivial rework / code cleanup without any test coverage.

Committer checklist

  • Has a corresponding JIRA in PR title & commit

  • Commit message is descriptive of the change

  • CI is green

  • Necessary doc changes done or have another open PR

  • For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

@hddong hddong marked this pull request as draft May 20, 2020 12:24
@bvaradar
Copy link
Contributor

@yanghua @leesf : Would you be interested in shepherding this PR when it is ready ?

@yanghua
Copy link
Contributor

yanghua commented May 21, 2020

you be interested in shepherding this PR when it is

Yes, of course.

@yanghua yanghua self-assigned this May 21, 2020
@yanghua yanghua self-requested a review May 21, 2020 02:13
@codecov-commenter
Copy link

codecov-commenter commented May 21, 2020

Codecov Report

Merging #1645 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1645   +/-   ##
=========================================
  Coverage     18.33%   18.33%           
  Complexity      855      855           
=========================================
  Files           344      344           
  Lines         15167    15167           
  Branches       1512     1512           
=========================================
  Hits           2781     2781           
  Misses        12033    12033           
  Partials        353      353           

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 6a0aa9a...5abfd69. Read the comment docs.

@hddong hddong marked this pull request as ready for review May 21, 2020 04:30
@hddong
Copy link
Contributor Author

hddong commented May 21, 2020

@yanghua : it's ready now.

Copy link
Contributor

@yanghua yanghua left a comment

Choose a reason for hiding this comment

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

@hddong Left some comments.

@@ -64,8 +64,9 @@ public void init() throws IOException {
tablePath = basePath + File.separator + tableName;

// Create table and connect
System.out.println(tablePath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove stdout print?

@Test
public void testWriteAmplificationStats() {
// generate data and metadata
Map<String, Integer[]> data = new LinkedHashMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

-> Map<String, Integer[]> data = new LinkedHashMap<>();

public void testFileSizeStats() throws IOException {
String commit1 = "100";
String commit2 = "101";
Map<String, Integer[]> data = new LinkedHashMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

return HoodiePrintHelper.print(header, getFieldNameToConverterMap(), sortByField, descending, limit, headerOnly, rows);
}

public Map getFieldNameToConverterMap() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we return a generic type Map<String, Function<Object, String>>?

@hddong
Copy link
Contributor Author

hddong commented May 21, 2020

@yanghua Thanks for your review, had address them.

Copy link
Contributor

@yanghua yanghua left a comment

Choose a reason for hiding this comment

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

LGTM. @hddong thanks for your contribution.

@yanghua yanghua merged commit 802d16c into apache:master May 21, 2020
@hddong hddong deleted the add-test-StatsCommand branch May 21, 2020 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants