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

need Integration Tests InterceptMessage cleanup #1338

Closed
plebhash opened this issue Jan 9, 2025 · 0 comments · Fixed by #1339
Closed

need Integration Tests InterceptMessage cleanup #1338

plebhash opened this issue Jan 9, 2025 · 0 comments · Fixed by #1339

Comments

@plebhash
Copy link
Collaborator

plebhash commented Jan 9, 2025

follow up to #1297

InterceptMessage::response_message field name

this name is a bit misleading, as this is not really a response, it's just the new message that will replace the old one

test_sniffer_interrupter is not comprehensive, and motivation for break_on is not clear

the name of this test implies that we are testing that the "interrupt" functionality (break_on: true) works as expected, but we never actually assert that the connection was terminated

we only assert that SetupConnectionSuccess was replaced with SetupConnectionError, so in the very least this test has a bad name

but I believe the problem goes a bit deeper

tbh looking at this test gives me the impression that we're kind of mixing "interrupt" + "intercept" terminology, which IMHO is something we need to be careful moving forward in order to further propagate this confusion

for clarity, here's my understanding of these terms:

  • "intercept": this is a very generic term... it simply means Sniffer took some message and it's about to do something with it (maybe just log it, maybe replace it with a different one)
  • "interrupt": it means that after some interception, the Sniffer is supposed to be killed

during this analysis, I also noticed that if break_on is true, recv_from_down_send_to_up and recv_from_up_send_to_down return a SnifferError::MessageInterrupted

this error is never checked nor used for anything, so what is supposed to happen after this error is returned? does Sniffer terminate its TCP connections? is it dropped from memory? currently, absolutely nothing happens, and we also don't have any test for that (despite the misleading name of test_sniffer_interrupter)

in other words: there's no real "interrupt" functionality, only a superficial appearance of one

which then makes me wonder: what is the point of this "interrupt" functionality in the first place? do we have some real demand for this?

it was introduced on #1228 where the goal was simply to allow for messages to be modified, but I cannot find any reference in our discussions to really justify this extra functionality

in fact, I found this question that I resolved without a proper answer (maybe we discussed it on Discord, or some call?)

so my impression is that this was introduced as a potential "nice-to-have", but it was never properly designed and it feels it's only polluting and adding complexity to the code without any real benefit

so IMHO we should make things simpler and drop this "interrupt" functionality, until we realize that we actually need it (and then properly design and test it)

another review point of #1228 that was not properly addressed:

#1228 (comment)

currently, test_sniffer_interrupter (which as stated above, has a misleading name) only has one single sniffer, and we only assert that it holds SetupConnectionError internally

but we do not assert that the downstream actually received this SetupConnectionError , which was the original motivation for my request for 2 sniffers on this test

so we're only partially testing things and this test does not really give us full confidence that this functionality is working as expected (message is replaced and fully delivered over the TCP connection)

@plebhash plebhash changed the title Integration Tests InterceptMessage cleanup need Integration Tests InterceptMessage cleanup Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant