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.

Sign up for GitHub

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

Jump to bottom

R4R: Code cleanup, take #1 #3553

Merged
merged 2 commits into from
Feb 8, 2019
Merged

R4R: Code cleanup, take #1 #3553

merged 2 commits into from
Feb 8, 2019

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Feb 8, 2019 •
edited
Loading

  • Code cleanup round.
  • Canonicalize export command.
  • Add support for DONTCOVER comment to signal per-file coverage report exclusion.
  • Circleci changes required to support DONTCOVER.
  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@alessio alessio force-pushed the alessio/code-cleanup-and-coverage-1 branch from 0f4156d to bc00666 Compare February 8, 2019 01:43
@codecov
Copy link

codecov bot commented Feb 8, 2019 •
edited
Loading

Codecov Report

Merging #3553 into develop will increase coverage by 4.15%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #3553      +/-   ##
===========================================
+ Coverage    57.08%   61.24%   +4.15%     
===========================================
  Files          190      184       -6     
  Lines        14982    13999     -983     
===========================================
+ Hits          8553     8573      +20     
+ Misses        5889     4887    -1002     
+ Partials       540      539       -1

@alessio alessio force-pushed the alessio/code-cleanup-and-coverage-1 branch from 8701b15 to 39e5529 Compare February 8, 2019 06:23
@alessio alessio changed the title WIP: Alessio/code cleanup and coverage 1 R4R: Code cleanup, take #1 Feb 8, 2019
@alessio
Copy link
Contributor Author

alessio commented Feb 8, 2019

IMveryHO this is not worth a mention in PENDING.md.

@alessio alessio requested a review from jleni February 8, 2019 06:26
jleni
jleni reviewed Feb 8, 2019
View reviewed changes
.circleci/config.yml Show resolved Hide resolved
client/keys/add.go Outdated Show resolved Hide resolved
@alexanderbez alexanderbez mentioned this pull request Feb 8, 2019
Closed
4 tasks
jackzampolin
jackzampolin approved these changes Feb 8, 2019
View reviewed changes
Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

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

Great! MOAR COVERAGE! LGTM 👍

@alessio alessio force-pushed the alessio/code-cleanup-and-coverage-1 branch from 8fb6af3 to e2d96a1 Compare February 8, 2019 18:37
@alexanderbez
Copy link
Contributor

failing test_cover :(

@jleni
Copy link
Member

jleni commented Feb 8, 2019

mm... this is new... interesting:

--- FAIL: TestBonding (4.83s)
Error Trace: lcd_test.go:644
Error: Not equal:
expected: 30000000.000000000000000000
actual : 30303030.303030303030303030

          	Diff:
          	--- Expected
          	+++ Actual
          	@@ -1,2 +1,2 @@
          	-(types.Dec) 30000000.000000000000000000
          	+(types.Dec) 30303030.303030303030303030

Test: TestBonding

@alexanderbez
Copy link
Contributor

@jleni yeah see #3559. I think @alessio will fix this.

@alessio alessio force-pushed the alessio/code-cleanup-and-coverage-1 branch from e2d96a1 to 6f59f14 Compare February 8, 2019 19:41
@cwgoes
Copy link
Contributor

cwgoes commented Feb 8, 2019

LGTM pending CI & a rebase.

Alessio Treglia added 2 commits February 8, 2019 13:04
add a bunch of tests, add DONTCOVER text tag
a4e3eac 
- Also fix flaky test (closes: #3559). Don't test values
  returned by queries since there's no way to query a
  specific height via REST.
GetTempDir -> NewTestCaseDir
5cb21f1
@alessio alessio force-pushed the alessio/code-cleanup-and-coverage-1 branch from 1885e3a to 5cb21f1 Compare February 8, 2019 21:10
@cwgoes cwgoes added this to the v0.31.0 (Launch RC) milestone Feb 8, 2019
@cwgoes cwgoes merged commit e7e3c32 into develop Feb 8, 2019
@cwgoes cwgoes deleted the alessio/code-cleanup-and-coverage-1 branch February 8, 2019 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackzampolin jackzampolin jackzampolin approved these changes

@jleni jleni jleni approved these changes

@cwgoes cwgoes Awaiting requested review from cwgoes

@ebuchman ebuchman Awaiting requested review from ebuchman

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.31.0 (Launch RC)
Development

Successfully merging this pull request may close these issues.
5 participants
@alessio @alexanderbez @jleni @cwgoes @jackzampolin