8000 Begin improving branch coverage by isaacbrodsky · Pull Request #223 · uber/h3 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Begin improving branch coverage #223

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
Merged
merged 3 commits into from
Apr 18, 2019
Merged

Begin improving branch coverage #223

merged 3 commits into from
Apr 18, 2019

Conversation

isaacbrodsky
Copy link
Collaborator

Begin adding additional branch coverage.

Now
Overall coverage rate:
  lines......: 100.0% (1866 of 1866 lines)
  functions..: 100.0% (163 of 163 functions)
  branches...: 92.9% (981 of 1056 branches)

Previous
Overall coverage rate:
  lines......: 100.0% (1866 of 1866 lines)
  functions..: 100.0% (163 of 163 functions)
  branches...: 92.0% (971 of 1056 branches)

A few things I noted:

  • Some branches may be unreachable, e.g. because you couldn't construct an H3Index with a resolution below 0. Similar for assertions.
  • LCov looks inside macros like isfinite, which is implementation defined and may contain branches. This can cause differences in reported coverage.
  • LCOV_EXCL_LINE doesn't also exclude the branch leading to it.

@coveralls
Copy link
coveralls commented Apr 17, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling b3db81e on isaacbrodsky:improve-branch-coverage into 8b93167 on uber:master.

nrabinowitz
nrabinowitz approved these changes Apr 17, 2019
View reviewed changes
Copy link
Collaborator
@nrabinowitz nrabinowitz left a comment

Choose a reason for hiding this comment

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

A few nits, but LG overall!

src/apps/testapps/testBBox.c Outdated
west.west += 0.1;

t_assert(bboxEquals(&bbox, &bbox), "Equals self");
t_assert(!bboxEquals(&bbox, &north), "Equals different north");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I'd have the assertion messages here express the desired behavior, e.g. Does not equal different north

src/apps/testapps/testCoordIjk.c Outdated
t_assert(_unitIjkToDigit(&outOfRange) == INVALID_DIGIT,
"Unit IJK out of range");
t_assert(_unitIjkToDigit(&unnormalizedZero) == CENTER_DIGIT,
"Unit IJK to zero");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Use a different assertion message from &zero, to explain how this test is different?

src/apps/testapps/testHexRanges.c Outdated
H3Index* allKrings = calloc(2 * (1 + 6), sizeof(H3Index));
err = H3_EXPORT(hexRanges)(withPentagon, 2, 1, allKrings);

t_assert(err != 0, "Error on hexRanges");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Just for clarity, Expected error on hexRanges?

@isaacbrodsky isaacbrodsky merged commit 7927107 into uber:master Apr 18, 2019
@isaacbrodsky isaacbrodsky deleted the improve-branch-coverage branch April 18, 2019 15:58
mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022
@isaacbrodsky
Merge pull request uber#223 from isaacbrodsky/improve-branch-coverage
eaf26b7 
Begin improving branch coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nrabinowitz nrabinowitz nrabinowitz approved these changes

@dfellis dfellis dfellis approved these changes

@jogly jogly Awaiting requested review from jogly

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@isaacbrodsky @coveralls @nrabinowitz @dfellis
0