Calculate PR-style diff #532
Conversation
|
Admin commands cheatsheet:
|
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
| from coverage_comment import subprocess as subprocess_module | ||
|
|
||
|
|
||
| def test_get_added_lines( |
There was a problem hiding this comment.
I don't believe this is strictly necessary as it's more testing git than get_added_lines. I'm happy to take it out.
| import pytest | ||
|
|
||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
This and below seems to be copied from test_main, right ?
Could you deduplicate it ? It's ok to move it here, but delete the corresponding copied code from test_main
There was a problem hiding this comment.
Good call. Removed from test_main
tests/integration/test_coverage.py
Outdated
| def _check_added_lines(): | ||
| added_lines = coverage.get_added_lines(git, "main") | ||
|
|
||
| assert added_lines == { | ||
| relative_file_path: list(range(7, 13)) # Line numbers start at 1 | ||
| } |
There was a problem hiding this comment.
I'd rather add 2 duplicated lines inside the test than an inner function (so as to make tests as easy to read as possible)
Also, I believe it's ok if you make it a single line:
assert coverage.get_added_lines(git, "main") == {
relative_file_path: list(range(7, 13)) # Line numbers start at 1
}though that's your choice :)
| from coverage_comment import subprocess as subprocess_module | ||
|
|
||
|
|
||
| def test_get_added_lines( |
| with: | ||
| # New lines are discovered by running a three-dot notation diff on base_ref...head. They must share a common | ||
| # ancestor. Setting fetch-depth to 1000 here should ensure that. | ||
| fetch-depth: 1000 |
There was a problem hiding this comment.
I'll look into the e2e tests to test this since I'm not entirely sure how the actions/checkout action handles this. I wonder if you can get away with not setting fetch_depth here since a git fetch is called later.
There was a problem hiding this comment.
Oooooh, that's the comment I missed that caused the issue. I'm sorry @stickperson I completely missed that 😅
|
I've pushed a PR that fixes pre-commit that was broken, and it wasn't your fault. |
|
I think if you rebase now, there's a good chance CI will be green :) |
|
Hey there :) Still interested in getting this merged ? |
|
Yes, apologies! I will take a look on Monday. |
|
No need for apology :) , I was just worried it slipped your mind, but I'm very fine with a "move slow and don't break people" approach :D |
stickperson
force-pushed
the
three-dot-comparison
branch
from
2519fe2 to
2e8dc23
Compare
stickperson
force-pushed
the
three-dot-comparison
branch
from
a0a9432 to
c0575ea
Compare
|
Updated! |
|
It seems this PR is causing issues, it's probably my fault, but I'll revert for now. |
|
I'm working on reapplying this on #573 |
Fixes #529