Make all networking code mockable#21878
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Strong Concept ACK: mockable means fuzzable. |
|
Concept ACK |
|
@vasild I think 96bcc72 and refactors around it could easily be merged on its own. |
0256cfd to
e68bc63
Compare
|
|
|
Very nice. Strong concept ACK. |
e68bc63 to
5772edb
Compare
|
|
5772edb to
ec5f887
Compare
|
|
ec5f887 to
a0b0030
Compare
|
|
a0b0030 to
6bed261
Compare
|
|
|
|
|
Should this be marked as draft since it is a parent PR? |
|
@vasild this may be able to be closed now that all of the child PRs have been merged. |
Given that the majority of commits in this pull haven't been merged, I don't think so. |
|
Could be, I am not sure. The reason I did not make it a draft is because it can be merged as it is - there is no danger that something bad will happen if it gets merged (assuming there are reviewers who ACK it). The reason I split it into smaller PRs is solely to ease review. Should I make it draft?
Right, there are two pieces left - #26312 which I just opened and the tests themselves (which depend on that PR, thus no separate PR for them yet). |
|
|
|
|
Peeking at the underlying socket file descriptor of `Sock` and checkig if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of testing/mocking/fuzzing. Instead use an empty unique_ptr to denote that there is no valid socket.
The socket is always valid (the underlying file descriptor is not `INVALID_SOCKET`) when `GetBindAddress()` is called.
The socket is always valid (the underlying file descriptor is not `INVALID_SOCKET`) when `ConnectSocketDirectly()` is called.
`Sock::Get()` was used only in `sock.{cpp,h}`. Remove it and access
`Sock::m_socket` directly.
Unit tests that used `Get()` to test for equality still verify that the
behavior is correct, indirectly, by testing whether the socket is closed
or not.
Now that all network calls done by `CConnman::OpenNetworkConnection()` are done via `Sock` they can be redirected (mocked) to `FuzzedSocket` for testing.
|
|
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
All the functional bits of this are now merged. Thank you all! This enables extending the test framework to be able to call any method, without worries that it may end up doing OS syscalls (try to create sockets, open connections, etc). A PR that does some of that is in #28584 (= last 4 commits from this PR). Closing as complete. |
This is a roadmap PR. It can be merged, but it can also be split into separate PRs and to get proper thorough review it is split.
Add wrapper methods to the syscalls
accept(),setsockopt(),getsockname(),bind(),listen()in theSockclass (e.g.Sock::Accept()). Those methods can be overriden (mocked) by unit tests (existent example inmaster) and by fuzz tests (existent example inmaster).Change everybody to use
Sockinstead ofSOCKET.Move the functionality of
CConnman::SocketEvents()to a mockable method of theSockclass.Already merged pieces of this PR
Current pieces of this for review