[SERVER-68723] align SessionWorkflowTest with mocking of events, not guessing states Created: 11/Aug/22  Updated: 29/Oct/23  Resolved: 26/Jan/23

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 6.3.0-rc0

Type: Improvement Priority: Major - P3
Reporter: Billy Donahue Assignee: Billy Donahue
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
depends on SERVER-67426 ServiceStateMachineTest: simplify fun... Closed
Problem/Incident
causes SERVER-73421 finish SessionWorkflowTest mocking im... Closed
Related
is related to SERVER-72939 Fix data-race in `SessionWorkflow` un... Closed
Backwards Compatibility: Fully Compatible
Sprint: Service Arch 2022-12-26, Service Arch 2022-08-22, Service Arch 2022-09-05, Service Arch 2022-09-19, Service Arch 2022-10-03, Service Arch 2022-10-17, Service Arch 2022-10-31, Service Arch 2022-11-14, Service Arch 2022-11-28, Service Arch 2022-12-12, Service Arch 2023-01-09, Service Arch 2023-01-23, Service Arch 2023-02-06
Participants:
Linked BF Score: 60

 Description   
  • SessionWorkflowTest has a confusing implementation because it's centered around "states", but its inputs are events (the virtual functions intercepted by mocks).
  • Rephrase the tests, particularly StepRunner-based suites, as a standard mock sequence. An expected event, and a scripted response, and then a sequence of such simple exchanges.
  • The use of a ProducerConsumerQueue is conceptually problematic. Replace this with a single optional<Event> guarded by a mutex and condvar. This constrains the possibilities and makes the test simpler to reason about.
  • Collapse the RequestKind and FailureCondition enums into one. They are the same thing: an encoding for a scripted response. Some are payloads (RequestKind) and others are Status or other error behaviors (FailureCondition). Code is much cleaner when these are one enum, and we don't have to carry two numbers around, one of this is only useful if the other is kNone. It's over-complicating a simple thing.
  • Continue the trend from SERVER-67426 of making callback-based mocks, but take even more functions out of the fixture and convert them into local logic in the mocks.
  • Make the mocks in the fixture's setUp, with private factory functions. Tests do not need to makeSession. They should get one for free from the fixture.
  • Compact notation for the StepRunner-based test specs.
  • Simplify the mock callback bodies to be one-liner calls to `_onMockEvent<Payload>(event)`. This replaces several sites at which the same exact code was replicated 4 times. Exploit the symmetry. They're all doing the same thing: notifying the main thread that an event has occurred, block waiting for that message to be received by the main test thread, and responded to, and then return that response through the mock function's return value.

Lots of misc other refactors fall out of this.



 Comments   
Comment by Githook User [ 26/Jan/23 ]

Author:

{'name': 'Billy Donahue', 'email': 'billy.donahue@mongodb.com', 'username': 'BillyDonahue'}

Message: SERVER-68723 SessionWorkflowTest refactor (fix and unrevert)
Branch: master
https://github.com/mongodb/mongo/commit/875e14f1adb92cb0ab0e8b2aaffa3c7f73d49e4a

Comment by Kaloian Manassiev [ 18/Aug/22 ]

This change has been reverted under this commit, so I am reopening the ticket.

Comment by Billy Donahue [ 17/Aug/22 ]

Commit: [SERVER-68723 refocus SessionWorkflowTest on events not states](https://github.com/mongodb/mongo/commit/261b4a8e28cc7daf57135bb0fb31148388f930ff)
Branch: `master`

Generated at Thu Feb 08 06:11:34 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.