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.

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

Testing fixes #86

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Testing fixes #86

wants to merge 4 commits into from

Conversation

fjarri
Copy link
Member

@fjarri fjarri commented Jan 10, 2025

  • Fixed an error message in ProtocolMessagePart::assert_is_none() - it was the opposite of what it should have really said
  • Cosmetic improvements in SessionReport::brief() - do not show parts of the report that have no entries
  • Switch to ordered delivery of messages in run_sync()

Some details on the latter. When writing tests for invalid echo messages, I noticed that the test sometimes fails without generating an evidence, but rather complaining that an unexpected message was encountered. The full sequence of events was the following:

  • A test overrides a round to send out an echo message, where normally a round doesn't send one;
  • Session for that node sees there's an echo message and assumes there's an echo round
  • It finalizes the round and sends out an echo message
  • Due to delivery being random, sometimes it gets delivered before the actual round message with the payload that would trigger the expected error
  • The other nodes do not expect an echo round, so they return an unprovable error about an unexpected message

It could be fixed by making the user explicitly declare if there's an echo round, regardless of there being an echo broadcast, but that means more boilerplate in every round declaration. Instead I chose to make the message delivery in run_sync() ordered (for each node separately), so that the main round message arrives first and triggers the expected error.

The unordered delivery is still valuable for manul self-testing, so it is left as an option (although currently not used).

@coveralls
Copy link

coveralls commented Jan 10, 2025

Pull Request Test Coverage Report for Build 12715704549

Details

  • 39 of 71 (54.93%) changed or added relevant lines in 3 files are covered.
  • 8 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.2%) to 69.329%

Changes Missing Coverage Covered Lines Changed/Added Lines %
manul/src/protocol/message.rs 0 1 0.0%
manul/src/dev/run_sync.rs 35 41 85.37%
manul/src/session/transcript.rs 4 29 13.79%
Files with Coverage Reduction New Missed Lines %
manul/src/protocol/round.rs 1 59.34%
manul/src/session/echo.rs 1 59.38%
manul/src/protocol/message.rs 1 78.38%
manul/src/dev/session_parameters.rs 2 80.56%
manul/src/session/message.rs 3 95.11%
Totals Coverage Status
Change from base Build 12623730993: -0.2%
Covered Lines: 1849
Relevant Lines: 2667

💛 - Coveralls

@fjarri fjarri requested a review from dvdplm January 10, 2025 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants