Skip to content

Conversation

@gsheni
Copy link
Contributor

@gsheni gsheni commented Jan 5, 2022

@gsheni gsheni changed the title Update conftest.py Skip code coverage for specific dask lines Jan 5, 2022
@gsheni gsheni self-assigned this Jan 5, 2022
@gsheni gsheni requested review from davesque and rwedge January 5, 2022 21:29
@gsheni gsheni marked this pull request as ready for review January 5, 2022 21:29
Comment on lines 1241 to 1242
except AttributeError: # pragma: no cover
cpus = psutil.cpu_count() # pragma: no cover
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can put # pragma: no cover at the top of any block and it prevents tracking coverage for all lines in that block. So you wouldn't need the pragma on line 1242. I think this also works for functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed:
4492edc

approximate='1 hour')


def test_warning_not_enough_chunks(pd_es, capsys, three_worker_scheduler):
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can put a single # pragma: no cover at the end of this line and avoid putting one on each line of the function body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed:
4492edc

Copy link
Contributor

Choose a reason for hiding this comment

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

Did this section get updated? I'm seeing that the function still has a pragma no cover on each line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, fixed:
a1ac50e

approximate='1 hour')


def test_warning_not_enough_chunks(pd_es, capsys, three_worker_scheduler):
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this section get updated? I'm seeing that the function still has a pragma no cover on each line.

@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #1829 (82baa04) into main (9af7410) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1829      +/-   ##
==========================================
+ Coverage   98.74%   98.79%   +0.04%     
==========================================
  Files         142      142              
  Lines       15841    15820      -21     
==========================================
- Hits        15642    15629      -13     
+ Misses        199      191       -8     
Impacted Files Coverage Δ
...computational_backends/calculate_feature_matrix.py 100.00% <ø> (+0.84%) ⬆️
featuretools/tests/conftest.py 100.00% <ø> (ø)
...utational_backend/test_calculate_feature_matrix.py 100.00% <100.00%> (+0.54%) ⬆️

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 9af7410...82baa04. Read the comment docs.

Copy link
Contributor

@rwedge rwedge left a comment

Choose a reason for hiding this comment

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

I think we should make an issue to figure out why sometimes the entire test_warning_not_enough_chunks is uncovered

es,
verbose=True)
assert False
assert False # pragma: no cover
Copy link
Contributor

Choose a reason for hiding this comment

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

can this line reasonably be hit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting we take out the line completely?

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be that this assertion was placed here to indicate that whatever comes before it is intended to cover all cases. I sometimes do this, for example, if I have a string of if statements that are meant to cover all the possible types of values that might be passed to a function. Then, I'll include a final else: that just has something like raise Exception("invariant") or assert False. It shows very clearly that the if statements were supposed to handle any kind of value that came in and that they should have explicitly listed out the supported value types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with pytest.raises:
82baa04

@gsheni
Copy link
Contributor Author

gsheni commented Jan 6, 2022

@rwedge Here is the created issue:
#1831

@davesque davesque self-requested a review January 6, 2022 01:55
davesque
davesque previously approved these changes Jan 6, 2022
Copy link
Contributor

@davesque davesque left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes! Looks good to me.

@gsheni gsheni enabled auto-merge (squash) January 6, 2022 21:31
@gsheni gsheni requested review from davesque and rwedge January 6, 2022 21:46
Copy link
Contributor

@rwedge rwedge left a comment

Choose a reason for hiding this comment

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

LGTM

@gsheni gsheni merged commit 8383c16 into main Jan 6, 2022
@gsheni gsheni deleted the add_no_coverage_dask branch January 7, 2022 17:00
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.

4 participants