clippy: elide needless lifetime

[adam-cattermole]

New lint in 1.83 (we're pinned to 1.75 locally iirc but will hit this on PR)

[adam-cattermole]

Looks like we're hitting the header re-ordering issue again in the integration tests... I'm wondering if this is in some way related to the cargo version as it passed 4 hours ago (on 1.82) and now fails (on 1.83).. I'll re-run a few times to check if it's failing consistently

[alexsnaps]

Wondering if we should not assert against None, instead of the Some(&[u8])... This really doesn't buy us much.
If we had tests that whatever creates the message, creates the appropriate one (rather than the wire representation), that's all we'd need... like... an Operation returning the message :P

[adam-cattermole]

I agree, and these have proven too much of a headache - I've removed in the latest commit.

In the refactor we should ideally use unit tests to check the messages (CheckRequest/RateLimitRequest) are created as expected, at which point checking these bytes is not overly valuable anyway. Curious on others thoughts though, @eguzki @didierofrivia happy with this (5f64ef5)?

[alexsnaps]

🙌

testing the wire format representation is rarely a good idea... some json library do reshuffle or otherwise slightly change the input purposefully to avoid having users doing these checks... e.g. {"foo": 1, "bar": true} is equivalent to { "bar": true, "foo" :1}. Set semantic is usually a good candidate for these assumption to break... as was the case here.

[eguzki]

n the refactor we should ideally use unit tests to check the messages (CheckRequest/RateLimitRequest) are created as expected, at which point checking these bytes is not overly valuable anyway

That's something I tried. It turns out that the crate type is crate-type = ["cdylib"] so integration test binaries cannot link to it. The workaround is to add yet another crate type lib, which would enable to do that. However, compilation would take double .

[alexsnaps]

That's something I tried. It turns out that the crate type is crate-type = ["cdylib"] so integration test binaries cannot link to it.

No, that's unrelated... what @adam-cattermole is saying, have these assertions in unit tests just like all the other unit tests that exist already, not in integration tests.

[adam-cattermole]

That's something I tried. It turns out that the crate type is crate-type = ["cdylib"] so integration test binaries cannot link to it.

I was more thinking unit tests not integration, where we test the parts of the application work without the "filter" bits

[eguzki]

happy with this (5f64ef5)?

TBH not really. What I want to assert is on the descriptor entries being sent to the wire. Obviously we are failing to do that now, so this change is just saving some headache.. but I wanted to highlight that this is still a tech debt.. "I want to assert that I send descriptor entries A1 == B1 and A2 == B2"

[alexsnaps]

"I want to assert that I send descriptor entries A1 == B1 and A2 == B2"

Right, and that doesn't need to be an integration test. This is currently tested for at the wrong abstraction layer.

[adam-cattermole]

What I want to assert is on the descriptor entries being sent to the wire

I guess what I'm suggesting is that if we can get to a point where we have a separation of the "filtercontext" code from the part that is processing an action/operation we can unit test that a message is being created correctly... rather than integration testing that it's being created and then serialized