You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
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)
The text was updated successfully, but these errors were encountered:
follow up to #1297
InterceptMessage::response_message
field namethis 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 forbreak_on
is not clearthe 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 terminatedwe only assert that
SetupConnectionSuccess
was replaced withSetupConnectionError
, so in the very least this test has a bad namebut 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:
Sniffer
took some message and it's about to do something with it (maybe just log it, maybe replace it with a different one)Sniffer
is supposed to be killedduring this analysis, I also noticed that if
break_on
istrue
,recv_from_down_send_to_up
andrecv_from_up_send_to_down
return aSnifferError::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 oftest_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 holdsSetupConnectionError
internallybut we do not assert that the downstream actually received this
SetupConnectionError
, which was the original motivation for my request for 2 sniffers on this testso 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)
The text was updated successfully, but these errors were encountered: