Add tests to support SensorReader refactoring #29
Conversation
All the other tests are targetting code in the pms package so this "pms" subpackage containing a single test file was confusing to me. Since the tests are primarily targetting code in the pms.cli package it makes sense to move the file up one level so it roughly matches the directory structure of the code under test. Having all the tests follow a consistent structure will make it easier to add new ones. Note: the existing tests are a mix of unit and integration isolations; this change just makes the existing structure a bit clearer without trying to introduce further separation of the tests by isolation level.
The test package name now matches that of the package that's being targetted by these tests, helping to make the test structure a bit clearer in advance of adding more tests.
This uses a mock_serial library to test the context manager makes the expected read/write calls to the underlying serial device. By specifying the sensor as "PMSx003" we can hard-code the necessary stubs so that the mock device interacts properly with the reader. While other tests cover parts of the reader [^1], much of the code is mocked using the "capture" fixture [^2]. This test adds some of the missing coverage before we change the class in later commits. Note that the tests can be brittle due to use of "sys.exit" in the reader code: if something goes wrong and the conditional branches with "sys.exit" run, pytest will terminate early and incorrectly show tests as passing. We'll try and improve this in a later commit. [^1]: https://github.com/benthorner/PyPMS/blob/d6e6333d861c6780ba2d232cc8d530d8c6693f39/tests/test_cli.py [^2]: https://github.com/benthorner/PyPMS/blob/d6e6333d861c6780ba2d232cc8d530d8c6693f39/tests/conftest.py#L110-L166
This will make it easier type functions that operate on a general reader object, such as in the next commits. It also clarifies conceptually what a reader is.
This is a step towards making the code in the reader module work in a library context, as well as making it possible to test the happy and unhappy paths reliably - without sys.exit interfering. In order to keep the same CLI behaviour - and not print a stacktrace - each command that calls the reader now needs to handle the error and call sys.exit(1) itself. Although it's duplicated, exiting the program at the top-level makes it easier to reason about.
|
Wow, really nice PR. I would like to look in more detail into the It should take me a day or two to find the time to look at your PR in more detail. Thanks again, |
|
@avaldebe thanks for taking a look 🙁. I'm pleased you're keen on Regarding the
It's possible to create a I think the following could help:
Doing (1) unblocks making I don't mind doing (1) and (2) as part of this PR. Doing (3) is too out-of-scope for me though. What are your thoughts? Footnotes
|
|
Hi @benthorner I think the best is to keep this PR as it is. The ABC parent class ( I'll merge your PR as it is. Please met me know what are you palling for your future PRs. Cheers, |
|
Oh nice thanks @avaldebe. I'll try and get the next PR up soon with the actual refactoring in it. The change shouldn't be that big so it's probably easier if I just do it and then send the PR your way so you can see. |
|
Edit: while refactoring I went on a tangent to improve the variable names in one of the tests; not important really but here's the PR if you'd like to consider it as an extension of this one. |
2 participants
This is the first part of a proposed change to refactor the
SensorReader class so it's more flexible to use in a library.
Although the test coverage was reported as 100%, this is
misleading as much of the SensorReader class is mocked
when tests are run [^1]. Adding tests around SensorReader
specifically means we can refactor with more confidence.
Since adding tests is a big change on its own, I thought it
would be best to suggest it first, before the actual refactor.