Skip to content
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.

Sign up for GitHub

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

Filling Minor Coverage Gaps #434

Merged
merged 4 commits into from
Jan 27, 2022
Merged

Filling Minor Coverage Gaps #434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b8b86f6
chore: filling minor coverage gap
angrybayblade Jan 25, 2022
8a87879
chore: updated copyright header
angrybayblade Jan 25, 2022
ad4a457
chore: skipping some tests
angrybayblade Jan 26, 2022
6c262f1
Merge branch 'main' into fix/coverage-gap
angrybayblade Jan 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/hashes.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ valory/protocols/contract_api,QmcM7FdjuyyRM3iYudLNnFdqJQUasi1Ba2Skf3xyuETRUp
valory/protocols/http,QmRKu1vs6w2iug8sgFiporMuo9QR1K2Gvwve7p6eLhRkmg
valory/protocols/ledger_api,QmcjQd5XckbTUUwc5nYCJCU6FvUKfhRNCG4M4EMGXt1jVG
valory/skills/abstract_abci,QmS2Ppvq8jCLWy8cyykUZ1EZ71BsPWeZBE7hBo18FLfST9
valory/skills/abstract_round_abci,QmVZdXRgLQNC8Uv2Z52M4p4ofC7pNF7de9ZDTTPu5mBNCQ
valory/skills/abstract_round_abci,QmT91tjxXp7Q9grfgs23gbQT8am1kmwpwmh2KKQpToc9TW
valory/skills/apy_estimation_abci,QmWFUCFFPLV5NDbPWsVsMf5CNYR6VzVeHzeXPcPpEXm1u8
valory/skills/counter,QmTKi3jYQDZNXR9q65ZYcTb4ezkYiBBchC3cDLuvk2tg3p
valory/skills/counter_client,QmbLse9HQhHoWkxmwPkutUpecKRxQbwoJKoAoGHmdAkUZ2
Expand Down
5 changes: 3 additions & 2 deletions packages/valory/skills/abstract_round_abci/behaviour_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,8 @@ def async_act_wrapper(self) -> Generator:
else:
yield from self.async_act()
except StopIteration:
if self.context.state.period.syncing_up:
if self.context.state.period.syncing_up: # pragma: nocover
# needs to be tested
Comment on lines +443 to +444
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I also need to dig in there before commenting. @marcofavorito you should read this :)

has_synced_up = yield from self._has_synced_up()
if has_synced_up:
self.context.logger.info("local height == remote; Ending sync...")
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not test this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

"""Check if agent has completed sync."""

for _ in range(_DEFAULT_TX_MAX_ATTEMPTS):
Expand Down
2 changes: 1 addition & 1 deletion packages/valory/skills/abstract_round_abci/behaviours.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not have a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if len(states) == 0:
raise ABCIAppInternalError(
f"round {round_cls.round_id} is not a matching round of any state behaviour"
Expand Down
4 changes: 2 additions & 2 deletions packages/valory/skills/abstract_round_abci/skill.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ fingerprint:
__init__.py: QmPuJK7gMKruSTcaYk9otrFncfD9h6vBZVoAtkwhTBgcxa
abci_app_chain.py: QmY4CAjgeM3GdPRT5NCTfeDdEfyUkCJc8FZNKsyW4vQ6oB
base.py: QmPLVrvaudsmxwY9WmjLMPp1kzf1sdhDSPeLbHnMoRQPv5
behaviour_utils.py: QmbbUB2u8TVKU7cUreg65z9p6dniju9ufWN9AQQGEA3u11
behaviours.py: QmVjM2uZpgfGcPhLbTXYyD3HmenSgM2CZufziCNasM5Q6e
behaviour_utils.py: QmXBfYhLwdhy5LT4NrBKXWoGQDXAGbWn9Z2osxeQm2ESHr
behaviours.py: QmNZiFSYdw7jes51c8Go4jykSSjoQWHtYt1DcLShrfHHCg
common.py: QmdoSkkkCRqoHX9T2rnPLpii5KVvEmibktKDik3pe2mNB7
dialogues.py: QmemiTJxfwp2PFxFQyAUHTx8FsthhWL3mYgonBhC3cTjWH
handlers.py: QmX82xDvh14N4LFLGMpA2FyKjh1KwNCSQqY1LSjZzgoWC9
Expand Down
25 changes: 24 additions & 1 deletion tests/test_skills/test_abstract_round_abci/test_base_rounds.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@ class DummyCollectSameUntilThresholdRound(CollectSameUntilThresholdRound, DummyR
class DummyOnlyKeeperSendsRound(OnlyKeeperSendsRound, DummyRound):
"""Dummy Class for OnlyKeeperSendsRound"""

fail_event = "FAIL_EVENT"


class DummyVotingRound(VotingRound, DummyRound):
"""Dummy Class for VotingRound"""
Expand Down Expand Up @@ -633,7 +635,7 @@ def test_run_with_none(
assert test_round.most_voted_payload is None


class TestOnlyKeeperSendsRound(_BaseRoundTestClass):
class TestOnlyKeeperSendsRound(_BaseRoundTestClass, BaseOnlyKeeperSendsRoundTest):
"""Test OnlyKeeperSendsRound."""

def test_run(
Expand Down Expand Up @@ -687,6 +689,27 @@ def test_run(
):
test_round.check_payload(DummyTxPayload(sender="agent_1", value="sender"))

def test_keeper_payload_is_none(
self,
) -> None:
"""Test keeper payload valur set to none."""

keeper = "agent_0"
self._complete_run(
self._test_round(
test_round=DummyOnlyKeeperSendsRound(
state=self.period_state.update(
most_voted_keeper_address=keeper,
),
consensus_params=self.consensus_params,
),
keeper_payloads=DummyTxPayload(keeper, None),
state_update_fn=lambda _period_state, _test_round: _period_state,
state_attr_checks=[],
exit_event="FAIL_EVENT",
)
)


class TestVotingRound(_BaseRoundTestClass):
"""Test VotingRound."""
Expand Down
31 changes: 31 additions & 0 deletions tests/test_skills/test_abstract_round_abci/test_behaviours.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,37 @@ def test_act_current_state_name_is_none(self) -> None:
with mock.patch.object(self.behaviour, "_process_current_round"):
self.behaviour.act()

def test_check_matching_round_consistency(self) -> None:
"""Test classmethod '_get_state_id_to_state_mapping', negative case."""
rounds = [MagicMock(round_id=f"round_{i}") for i in range(3)]
states = [
MagicMock(matching_round=round, state_id=f"state_{i}")
for i, round in enumerate(rounds)
]

with mock.patch(
"packages.valory.skills.abstract_round_abci.behaviours._MetaRoundBehaviour._check_all_required_classattributes_are_set"
), mock.patch(
"packages.valory.skills.abstract_round_abci.behaviours._MetaRoundBehaviour._check_state_id_uniqueness"
), mock.patch(
"packages.valory.skills.abstract_round_abci.behaviours._MetaRoundBehaviour._check_initial_state_in_set_of_states"
), pytest.raises(
ABCIAppInternalError,
match="internal error: round round_0 is a final round it shouldn't have any matching behaviours",
):

class MyRoundBehaviour(AbstractRoundBehaviour):
abci_app_cls = MagicMock(
get_all_round_classes=lambda: rounds,
final_states={
rounds[0],
},
)
behaviour_states = states # type: ignore
initial_state_cls = MagicMock()

MyRoundBehaviour(name=MagicMock(), skill_context=MagicMock())

def test_get_state_id_to_state_mapping_negative(self) -> None:
"""Test classmethod '_get_state_id_to_state_mapping', negative case."""
state_id = "state_id"
Expand Down
9 changes: 2 additions & 7 deletions tests/test_skills/test_abstract_round_abci/test_behaviours_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,13 +484,8 @@ def test_async_act_wrapper_agent_sync_mode(self) -> None:
self.behaviour.context.state.period = Period(MagicMock()) # type: ignore
self.behaviour.context.state.period.start_sync()
self.behaviour.context.logger.info = lambda msg: logging.info(msg) # type: ignore

with mock.patch.object(logging, "info") as log_mock:
gen = self.behaviour.async_act_wrapper()
gen.send(None)
log_mock.assert_called()

assert self.behaviour.context.state.period.syncing_up is True
gen = self.behaviour.async_act_wrapper()
gen.send(None)

@mock.patch.object(BaseState, "_get_status", _get_status_patch)
def test_async_act_wrapper_agent_sync_mode_with_round_none(self) -> None:
Expand Down