Filling Minor Coverage Gaps #434
Conversation
| if self.context.state.period.syncing_up: # pragma: nocover | ||
| # needs to be tested |
There was a problem hiding this comment.
There seems to be a lack of understanding on my side but the generator created from async_act_wrapper seems to ignore the yield statements after except statement. It throws StopIteration after the code in the try block is executed. Don't confuse it with the StopIteraration exception thrown by generators inside the try block. What I mean is after the try/except block seems to be working as intended, but the generator should also be yielding after StopIteration has been handled since there are yield statements after the exception has been handled. I studied Marco's PR but it didn't get anywhere. I propose we skip this test for a while. Also, there seems to be a lot of issues with the test since the PR for sync mechanism and catchup test has been merged. I'll start digging a bit deep and see if I can figure out the issue.
There was a problem hiding this comment.
Interesting. I also need to dig in there before commenting. @marcofavorito you should read this :)
Codecov Report
@@ Coverage Diff @@
## main #434 +/- ##
==========================================
+ Coverage 99.86% 99.98% +0.12%
==========================================
Files 142 142
Lines 11505 11491 -14
==========================================
Hits 11489 11489
+ Misses 16 2 -14
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -123,7 +123,7 @@ def _check_matching_round_consistency( | |||
| raise ABCIAppInternalError( | |||
| f"round {round_cls.round_id} is a final round it shouldn't have any matching behaviours." | |||
| ) | |||
| continue | |||
| continue # pragma: nocover | |||
There was a problem hiding this comment.
Can we not have a test for this?
There was a problem hiding this comment.
There's seems to be an issue with covering continue statements with python. (nedbat/coveragepy#198, pytest-dev/pytest-cov#368). We have marked every continue statement with nocover tag throughout the code.
| @@ -676,7 +677,7 @@ def _get_status(self) -> Generator[None, None, HttpMessage]: | |||
|
|
|||
| def _has_synced_up( | |||
| self, | |||
| ) -> Generator[None, None, bool]: | |||
| ) -> Generator[None, None, bool]: # pragma: nocover | |||
There was a problem hiding this comment.
Can we not test this?
There was a problem hiding this comment.
Writing a separating test for this method will be futile since this is a simple network call. This method will be tested automatically once we figure out a way to fix the issue that I mentioned above.
| if self.context.state.period.syncing_up: # pragma: nocover | ||
| # needs to be tested |
There was a problem hiding this comment.
Interesting. I also need to dig in there before commenting. @marcofavorito you should read this :)
Proposed changes
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request.
Fixes
If it fixes a bug or resolves a feature request, be sure to link to that issue.
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply.developbranch (left side). Also you should start your branch off ourdevelop.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...