test(api): Test coverage for WebSocket server

[slowli]

What ❔

Cover basic WS-specific JSON-RPC functionality (subscriptions, getFilterChanges) with tests.

Why ❔

Will allow to set the expectations the current server behavior, so that it could be evolved in the future w/o unexpected / breaking changes (e.g., when implementing jsonrpsee-based subscriptions).

Checklist

• PR title corresponds to the body of PR (we generate changelog entries from PRs).
• Tests for the changes have been added / updated.
• Code has been formatted via zk fmt and zk lint.

[slowli]

@montekki I haven't managed to find a way to reproduce duplicating or dropping event notifications so far. The notification logic, while not always straightforward, seems correct. If you have an idea how to reproduce the bug, please share.

[RomanBrodetski]

Was only able to have a quick look and it looks good. @slowli please let me know if you want me to have a close look at any particular part!

[slowli]

@RomanBrodetski Please take a look at FIXMEs in tests. They are related to the getFilterChanges implementation, which (as we've briefly discussed elsewhere) may return the same updates multiple times. (Since the method it uses is modifying the filter cursor, and the HTTP / WS transport cannot be assumed to be reliable, we have neither "at most once" nor "at least once" semantics for update delivery.)

[montekki]

@slowli Why again are getFilterChanges tests in the websocket testing? I believe that these methods are not websocket-specific and should instead go into main http endpoint testing. Only eth_subscribe is websocket-specific. If you agree I propose to move these tests into #546 and try to trigger all the corner cases there?

[slowli]

@montekki

I believe that these methods are not websocket-specific and should instead go into main http endpoint testing

They are indeed not WS-specific, although in practice it's impossible to use getFilterChanges HTTP endpoint for our testnet / mainnet deployments because of K8s load balancing (subsequent queries may arrive at different pods that don't have the referenced filter installed).

As for your proposal: Could it make sense to move tests within this PR (just make them test the HTTP server)? If this PR is merged before #546, then that PR could update the tests to reflect corrected server behavior.

[montekki]

As for your proposal: Could it make sense to move tests within this PR (just make them test the HTTP server)? If this PR is merged before #546, then that PR could update the tests to reflect corrected server behavior.

Yeah good we could do two things I think

1. either just move the abovementioned fix PR in here and expand the tests a bit
2. or merge this one with referencing the fix PR in the FIXMEs

Whatever you prefer