-
Notifications
You must be signed in to change notification settings - Fork 16
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.
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
Write-test-suite-#11 #39
Conversation
I think we may be stepping on each others toes a bit here. The URL generation code is thoroughly refactored and tested #36 🫠 Im on my cell phone but will give a closer look when im on my computer. |
@jordan-gillard no stress on my end so long as the unit tests are done by someone :) would appreciate a look over whenever you have time. |
Took a quick look over @jordan-gillard 's work on #36 - I think those unit tests look good and they will render some of what I have here redundant. The only value some of my tests here would add are 1) testing the RSS functions also and 2) covering the code that comes after the URL makes the call. I'll leave it to @GalenReich to look things over and let us know their thoughts, but by way of a constructive proposal, I could offer to wait until 36 is merged, then rebase this branch, adjust things, and resubmit keeping only the tests that actually add additional value as described above. I'll even throw in changing my "Arrange, Act, Assert" comments to "Given, When, Then" because consistency is king ;) |
results = list(call_args[0][0]) | ||
|
||
# ASSERT: Check if 'root_form' is present in the first result | ||
assert 'root_form' in results[0][0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is results[0][0]
, and what is the significance of root_form
?
What do you think of creating an end-to-end test that searches over a given time period and asserts the exact value written to the patched file?
With /pr-40 done, I'll emerge from hibernation and take a look at whether we need any more tests as soon as I can. |
I'm going to close in favour of #45 . Sorry for the confusion, was easier/cleaner than rebasing. |
Thanks for the patience on writing this. This is a starting point for a test suite. One challenge was how to make the tests resilient in the event that the SEC and RSS APIs change, or the return payload changes, or the tests are run from a machine without internet access. For this reason, I developed a pattern such that some tests use try-except to try and run an end-to-end test (which includes making a call to the web) and raises a warning, rather than a test failure, if the end-to-end test fails.
I also make use of the patch decorator to suppress behavior like file creation and web calls during testing.
One final issue is that I could not run @jordan-gillard's unit test
tests/test_cli.py::test_cli_should_return_help_string_when_passed_no_args
It seems something about my machine set up (Windows 11) doesn't work with using the subprocess module, but I cannot figure out what and I am using a python 3.11.2 poetry shell as per the instructions. Sorry if I'm missing something obvious but I can't get this one to pass :S . If this works on a testers computer, then perhaps it's just me(?). Because the behavior tested is about the CLI, it makes sense to use subprocess.
All other tests are passing with both pytest and tox. Test coverage is 69% (which is closer to 75% when not counting io.py, which I wanted to avoid testing)
Review and feedback much appreciated. @jordan-gillard and @rkiddy are very welcome to look it over and offer any feedback.
Cheers