Convert some tests to pytest #1693
Conversation
There was a problem hiding this comment.
For your test plan, include the steps you ran to make sure that things worked - in this case it's straightforward, I'd accept both
Ran py.test on updated tests
and
CI will show new tests work
I think we can clean up some of the extra features a little, comments inline
| @pytest.fixture | ||
| def file_content(): | ||
| """ | ||
| Fixture to open and yield file content for testing, | ||
| then close the file after the test. | ||
| """ | ||
| with open(TEST_FILE, "rb") as f: | ||
| yield f.read() |
There was a problem hiding this comment.
I am not sure that you need a fixture of this - the test can just open itself.
Fixtures are helpful when you are sharing setup between tests, and here's there is only one test.
| return [ | ||
| ("a", "a"), | ||
| ("a ", "a"), | ||
| ] | ||
|
|
||
| def get_compare_hash_cases(self): | ||
| @pytest.fixture |
There was a problem hiding this comment.
blocking: This is definitely doesn't need a fixture! You can just use a list!
There was a problem hiding this comment.
Any response on this unaddressed blocking comment?
| @pytest.fixture | ||
| def validate_hash_cases(self): |
There was a problem hiding this comment.
blocking: This doesn't look like it needs to be a fixture - it looks like a better fit for pytest.mark.parametrize
There was a problem hiding this comment.
Any response on this unadressed blocking comment?
| return [] | ||
|
|
||
| def get_matches_str_cases(self): | ||
| @pytest.fixture |
There was a problem hiding this comment.
blocking: Ditto that this looks like a better match for pytest.mark.parametrize
There was a problem hiding this comment.
Any response on this unaddressed blocking comment?
| @@ -1,21 +1,14 @@ | |||
| # Copyright (c) Meta Platforms, Inc. and affiliates. | |||
|
|
|||
| import unittest | |||
|
|
|||
| import pytest | |||
| assert UrlMD5Signal.hash_from_str(URL_TEST) == URL_TEST_MD5, "MD5 hash does not match" | ||
|
|
||
| class UrlMD5SignalTestCase(unittest.TestCase): | ||
| def test_can_hash_simple_url(self): | ||
| assert URL_TEST_MD5 == UrlMD5Signal.hash_from_str( | ||
| URL_TEST | ||
| ), "MD5 hash does not match" | ||
|
|
||
| def test_can_hash_full_url(self): | ||
| assert URL_TEST_MD5 == UrlMD5Signal.hash_from_str( | ||
| FULL_URL_TEST | ||
| ), "MD5 hash does not match" | ||
| def test_can_hash_full_url(): | ||
| assert UrlMD5Signal.hash_from_str(FULL_URL_TEST) == URL_TEST_MD5, "MD5 hash does not match" |
There was a problem hiding this comment.
note: The existing asset comments don't seem to add much, but I think it's fine to ignore
| ), | ||
| ], | ||
| ) | ||
| @pytest.fixture |
There was a problem hiding this comment.
ignorable: While this might be a misuse of feature, fixtures are basically a 1:1 mapping for setUp, so I think this a faithful translation
|
@Dcallies could you check that PR? I have fixed the requested changes |
There was a problem hiding this comment.
Hey @ZeyadTarekk, a few things, you said that you addressed all the feedback. However, I note that there are comments prefixed with "blocking:" which appear unchanged, and so are unaddressed. If you think that there is something I am missing and you think the code should actually be changed, you need to explain why that is. Otherwise, I will send it back to you for unaddressed blocking comments.
I note the presence of unaddressed blocking comments, and so I am sending it back to you to address them with code changes, or respond using github comments with why they should not be addressed. Can you do that, and once you have done so, hit "Request Review"?
| return [ | ||
| ("a", "a"), | ||
| ("a ", "a"), | ||
| ] | ||
|
|
||
| def get_compare_hash_cases(self): | ||
| @pytest.fixture |
There was a problem hiding this comment.
Any response on this unaddressed blocking comment?
| @pytest.fixture | ||
| def validate_hash_cases(self): |
There was a problem hiding this comment.
Any response on this unadressed blocking comment?
| return [] | ||
|
|
||
| def get_matches_str_cases(self): | ||
| @pytest.fixture |
There was a problem hiding this comment.
Any response on this unaddressed blocking comment?
Summary
Fixes #1686
Test Plan
Fixes #1686