Increased test coverage #29
Conversation
| spider = Spider("http://http.error") | ||
|
|
||
| resp404 = spider.fetch_url("http://http.error/404") | ||
|
|
There was a problem hiding this comment.
This test is too long.
-
We can break this based on SRP. like
test_fetch_url_handles_404_error,test_fetch_url_handles_408_erroretc. -
We can extract out common code
def setup_mock_response(url, status, body="<html><body><a href='http://http.error'>link</a></body></html>")and re use it. -
type: ignore (can we use type hints here. Maybe
pytest.CaptureFixture?)
There was a problem hiding this comment.
Yeah you're right I'll shorten it 👍
I don't understand what you mean in the 3rd thing tho. Are you referring to the places where I wrote # type: ignore? That's because otherwise mypy would complain about the capsys argument, which is only passed when running the tests.
There was a problem hiding this comment.
Just pushed the changes let me know what you think 👍
Awesome! Can we display it in Readme? Ideally we can make it like: If it falls below a threshold coverage (80%) the CI should fail. We can spec out that in a different issue. |
|
I don't know about making the ci fail when its bellow a certain coverage percentage, but I can add a badge to the readme with the coverage percent, which then links to the full report. |
| body=requests.exceptions.Timeout(), | ||
| status=408 | ||
| ) | ||
|
|
There was a problem hiding this comment.
responses.add(
responses.GET,
"http://timeout.error",
body=requests.exceptions.Timeout(),
status=408
)This is getting repeated in every test case. We can move this into a reusable function. And maybe move it to test/utils.py so that we can re use this other places as well.
something like this
@contextmanager
def capture_output(capsys: pytest.CaptureFixture) -> Generator[None, None, str]:
yield
captured = capsys.readouterr()
return captured.out
def setup_mock_response(url: str, status: int, body: str = "<html><body><a href='http://http.error'>link</a></body></html>") -> None:
responses.add(
responses.GET,
url,
body=body,
status=status
)and we can call the tests like this
@responses.activate
def test_fetch_url_handles_404_error(capsys: pytest.CaptureFixture) -> None:
setup_mock_response("http://http.error/404", 404)
spider = Spider("http://http.error")
with capture_output(capsys) as output:
resp404 = spider.fetch_url("http://http.error/404")
assert And repeat this patten for testing other exceptions.
Or even better to use parametrize fixtures
@pytest.mark.parametrize("url, status", [
("http://http.error/404", 404),
("http://http.error/408", 408),
("http://http.error/403", 403),
])
def test_fetch_url_http_errors(mock_responses, capsys_output, url, status):LMK your thoughts.
There was a problem hiding this comment.
Oh I get it I didn't understand it was meant to be for every test, give me a second and I'll implement it.
I've never used pytest parametrize so I might not be the most indicated to implement that, maybe we can just open an issue for it and it can be dealt with later?
There was a problem hiding this comment.
It is very simple. Basically instead of calling test_fetch_url_handles_404_error, test_fetch_url_handles_408_error we generalize it into one test test_fetch_url_http_errors and pass the fixture parms to it with the url to call and the response we expect. You can try it. If not working. I can pick it up later as part of the refactoring I am currently doing.
There was a problem hiding this comment.
It seems like capsys doesn't capture the stdout from inside the context manager.
There was a problem hiding this comment.
Ok. LMK once you fix other things and we can merge this.
tests/test_crawler.py
Outdated
| @responses.activate | ||
| def test_crawl_found_duplicate_url() -> None: | ||
| responses.add( | ||
| responses.GET, |
There was a problem hiding this comment.
We can reuse the helper setup_mock_response in all these places
There was a problem hiding this comment.
@indrajithi I added the helper function everywhere, I can't get the context manager to work. It seems like capsys doesn't capture the stdout from inside the context manager.
There was a problem hiding this comment.
Should I open an issue for it? To create the context manager for the tests?
There was a problem hiding this comment.
No issues. Let us ignore it for now. This looks good. We can pick displaying coverage in readme in a separate issue. I will merge this one.
There was a problem hiding this comment.
No issues. Let us ignore it for now. This looks good. We can pick displaying coverage in readme in a separate issue. I will merge this one.
I've already created an issue for it if you can just assign me there :) #30
Related to #28 and #24
Changes
Before, the test coverage was at around 79% (this is including the soon to be removed
mainfunction incrawler.py)I've added new test cases to
test_crawler.py, so that now the test coverage is at 100% (excluding themainfunction)I've also changed the ci workflow so that a coverage report is shown when running the tests, using the already installed
pytest-covextensionAnd I've also fixed some formatting issues present before my changes (not sure how they were passing the ci but they were getting found on my machine on the commit hook so I fixed them)