Refactor clproto tests #75
Conversation
| clproto::test_encode_decode<Parameter<TypeParam>>( | ||
| param, clproto::PARAMETER_MESSAGE, test_parameter_equal<Parameter<TypeParam>>, std::get<1>(this->test_case_)); | ||
| param.reset(); | ||
| // FIXME(#50): Encoding of empty parameters is incorrect |
There was a problem hiding this comment.
In order to make this work, we'll need another PR. I already have the fix but I wanted to create the test first and then make it work
| parameter = sr.Parameter(name, value, parameter_type) | ||
| assert_encode_decode(parameter, clproto.MessageType.PARAMETER_MESSAGE, helpers.assert_parameter_equal, message_type) | ||
| parameter.reset() | ||
| # FIXME: empty parameters that are encoded and decoded are not empty anymore! |
Add templated function to do the encode/decode routine that is common to all clproto tests.
There was a problem hiding this comment.
This is great 🔥 Well done leveraging the typed test and parameterized test fixtures. I always appreciate when we can apply learnings from later projects back to our early code to keep things up to a high standard.
I also definitely agree with the FIXMEs deferred to a later issue.
I also wanted to say, I kept an eye on the clock for this one and your ETR was spot on ;)
protocol/clproto_cpp/test/tests/include/test_clproto/encode_decode_helper.hpp
Outdated
Show resolved
Hide resolved
domire8
force-pushed
the
feature/clproto-tests
branch
from
792a84c to
9858a39
Compare
|
I had to rebase and push again anyway, so I included your nitpicks |
| @@ -0,0 +1,41 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
it's a nitpick on a nitpick but my written suggestion was to name the file after the function, test_encode_decode.hpp
Description
There is no real parent issue for this but it explains why the two issues above were opened in the first place. Let me try to explain:
ParameterContainerclass in Python were not correct (hence the rest of the parameter issue)There are a lot of changes, sorry for that. Review by commit
conftest.pythat will be useful for all other test modules too. They compare the different types of state representation classes.Review guidelines
Estimated Time of Review: 40 minutes