Feature/fix tests #21
Conversation
|
|
||
| EMPTY_RES = b'GETMEAS \r\n\r\n\r>' | ||
|
|
||
| @patch('serial.Serial', Mock(return_value=SERIAL_REPR)) |
There was a problem hiding this comment.
I like the spirit of testing everything here. This feels a little contrived, and I'm not sure how much it is really testing. Maybe some comments here about what this means and why we're doing it? Many of the readers/potential extenders of this code aren't going to be used to tests at all, much less this level of Mocking, maybe this is a chance to educate/guide?
There was a problem hiding this comment.
Addressed, gave explanation and included link to unittest docs
| def test_build_serial_command_A215(mock_init) -> None: | ||
| mock_init.return_value = None | ||
| def test_open_serial_port_failure_A215() -> None: | ||
| ph_meter = orion_star.OrionStarA215() |
There was a problem hiding this comment.
this is assuming the A215 isn't plugged into the default serial port, I guess? What if user does have it plugged into at default location and runs tests?
There was a problem hiding this comment.
I think we would mock out the response to mimic it not being plugged in? Because unit tests shouldn't depend on any hardware
There was a problem hiding this comment.
Agreed, I changed this to be based on trying a known bad port address. I looked into mocking the actual failure response but it gets complicated considering serial opens ports differently based on the operating system. Figured this would be an acceptable proxy...
|
|
||
| res = ph_meter.get_measurement() | ||
| assert isinstance(res, dict) | ||
| assert res == {"pH": "4.61", "mV": "111.2", "temp": "25.0"} |
There was a problem hiding this comment.
This is a useful, good test! But again, might be a chance to really describe what's going on so folks who are new to this can learn how this works.
| def test_check_response_failure_A215() -> None: | ||
| with pytest.raises(IndexError): | ||
| ph_meter = orion_star.OrionStarA215() | ||
| res = ph_meter._check_response(EMPTY_RES) |
There was a problem hiding this comment.
this EMPTY_RES is a great one to test for. Could this really happen?
Might be worth doing a few failure tests that might really happen. The meter gets unplugged halfway thru a read, for instance. Or maybe there is a noise source right next to cable, an otherwise good response gets a few characters jumbled, or some bad data inserted.
There was a problem hiding this comment.
This does happen occasionally, yes -- I added a few more test cases to handle random characters.
| def test_check_response_failure_norgren() -> None: | ||
| with pytest.raises(IndexError): | ||
| pump = norgren.VersaPumpV6() | ||
| res = pump._check_response(EMPTY_RES) |
There was a problem hiding this comment.
could we add """docstring comment explaining tests""" to the test cases so that users can quickly see what we're trying to test for in each scenario?
There was a problem hiding this comment.
Love all the new tests... overall I think comments around what each test is trying to check for would be super helpful. Also, how is the test suite run? We should probably try to add a github action to run the tests before merging any PR to make sure we don't merge broken code too
…pump error handling to be more explicit
added an action to run pytest. |
|
Addressed all previous comments -- let me know what you guys think! |
| # mocking the serial_port.read_until method allows injection of the | ||
| # expected serial response from a piece of hardware without needing to | ||
| # have the hardware present and operational, such that the testing of | ||
| # the code is separated from the operation of the hardware. |
Much more robust testing of pH and pump modules, everything to 100% coverage except the UI (which is a whole can of worms to try to test properly). Overall repo coverage is now about 80%.