Unit testing & review comments #3608
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PBENCH-1315 The index-map fix was submitted without unit tests so we can get it deployed to resolve the immediate PostgreSQL resource problems. This follow-on adds unit testing (and some late review comments). __NOTE__: this isn't complete, but it's far easier to get a coverage report from the CI than locally, so I'm posting a draft, which I'll be updating as I build test cases.
dbutenhof
added
Server
Unit tests
API
|
Ugh:
|
The good news is, that only happens on the first build.... |
This'll give some coverage of all three cases, but all based on the theory of "call Elasticsearch which returns nothing", which isn't realistic. I'm just experimenting with what I can twist out of the `Common` baseclass here, which isn't really suited for this API but has enough flexibility (I think) to "make do".
webbnh
reviewed
Feb 5, 2024
This proved an "interesting" journey. I found several returns that don't really make sense: for example, the old delete/update logic reported a `Sync.update` failure as `CONFLICT` rather than `INTERNAL_SERVER_ERROR`. The fixture we use install the Elasticsearch mock response and call the API needs to know whether to install the mock: it has to be right as either installing the mock and not calling it or calling it without the mock will result in failure. The current logic around installation of the mock proved difficult to reconcile with the need to handle `INTERNAL_SERVER_ERROR` both *before* and *after* the call to Elasticsearch. As a result, I ended up massively refactoring the `query_api` fixture code to simplify the logic based on a three-state override (neutral, force, or suppress) in the new `expect_call` parameter.
Do we have 100% *now*??
I fixed up mocking to properly unit test the `GET`, and realized that in the current simplistic form, the output wasn't very useful as it wouldn't capture the Elasticsearch `_index` value. A more useful output for a general `GET /datasets/<id>` would be a summary rather than returning full documents. So I rewrote the code and adjusted the unit tests. Incidentally, after the production server PostgreSQL recovery, I found that my report generator was tripping over datasets which had no operational status: I suspect this was due to some hole in intake (although I think that failure to create the `UPLOAD` operational status should have failed the intake, I'm not going to debug that today). I did, however, adjust the report generator to detect and report this case gracefully instead of failing with an f-string formatting error. I also added a summary line of rows, tables, and size for the SQL report.
|
I could maybe throw in a few more mock cases but basically there are three realistically unreachable edge cases left uncovered and making them fire would be messier than I want to try to justify right now. We're at 97% line coverage and 95% conditional coverage, which is pretty good. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
webbnh
approved these changes
Feb 20, 2024
More like, I read your comments and my brain got locked into these PRs so the only way past was through ... 😆 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PBENCH-1315
The index-map fix was submitted without unit tests so we can get it deployed to resolve the immediate PostgreSQL resource problems. This follow-on adds unit testing (and some late review comments).