pbt: convert gpb-eqc to PropEr & add to test suite #142
Conversation
Avoid line break when the type of the field is a message:
Previously:
Warning: ignoring option packed for non-packable field m.a of type {msg,
n}
Now:
Warning: ignoring option packed for non-packable field m.a of type {msg,n}
Previously message translations were only for google.protobuf.Any, and this was reflected in variable names and function names. Now, it these translations can be used for messages, this change names accordingly.
Instead of computing which translator helpers to generate, just generate
all helpers and-compile({nowarn_unused_function, helper/n}). attributes.
The calculation of which translator helpers to generate was a bit
convoluted, and could also produce questionable results, so ridding the
code of this was a net benefit.
This is a prerequisite to be able to translate map<_,_> type fields.
Pass any UserData for translations to all generated verification helper functions, instead of computing when it is needed. This is also a preparation for being able to translate any type, not only messages.
This is a preparation to make it possible to translate all types
This amounted to doing it for packed fixlen fields.
Also simplify by:
* passing TrUserData along to all helper functions
* generate -compile({nowarn_unused_function, helper/n})
instead of computating when helpers are actually needed
(this has been tricky business in the past)
This is a starting point for more thorough testing. I intend on extending https://github.com/tomas-abrahamsson/gpb-eqc with PropEr's Targeted testing properties.
...instead of exactly 21.0
Use a ?f(Fmt,Args) macro to still get compiler warnings for formatting issues, yet still fit 80.
...so it also works well in my emacs workflow
|
Noce! I'll be pushing a few additions. Rudimentary Makefile support because I find I like Among the changes are also a check for OTP >= 21.0 instead of exactly 21.0, hope travis likes my shell script, but knowing how difficult it can be to get all details right, I guess might have to push more than once. |
When an enum is encoded in proto3, if it is the default, it must not be included in the result. The first enum is the enum's default, and that enum must also be 0. Previously the default value handling was done only when the enum was set as the atom, but now it is also done when set as the integer value.
|
LGTM. Do you need anything from me? |
Limitation: messages still cannot be translated on top-level. Add test for translations for all scalar types Add command line option parsing for types
If a field is
message M {
oneof choice {
uint32 field alt1 = 1;
...
}
}
then the translate_field option can now operate on the oneof 'choice'
level, ie not only on the fields inside the oneof:
{translate_field,
{['M',choice],
[{encode,{M,F,Argtemplate}},
{decode,{M,F,Argtemplate}},
{verify,{M,F,Argtemplate}}]}}
Add the possibility to specify MsgName, even when generating code for records. This is a precursor to support top-level message translations.
|
I plan to include this and some more in an upcoming release. Would you know by the way, is there a way with rebar to make it run all tests except prop_gpb? About the travis job: why not run these tests for more versions of Erlang? |
There is unfortunately no option!
No reason to. Running against more versions of protobuf is needed though ( |
Make lookup functions common between translate_field and translate_type options.
|
For protobuf v3.* this can work: protoc() {
docker run -it --rm -v "$PWD":/w -w /w alpine /bin/sh -c 'apk update && apk upgrade && apk add protobuf && protoc '"$@"
} |
If using make and setting ERLC_FLAGS, then force -Iinclude to ensure the generated descriptor sources compile.
|
(I added #144 to track update libprotobuf.) I'm a bit concerned with two issues:
|
| # proper-test target download dir in the Makefile | ||
| /.deps | ||
| # when proper is fetched as a rebar dependency | ||
| /rebar.lock |
There was a problem hiding this comment.
Not sure why you'd do this: test dependencies are not locked.
There was a problem hiding this comment.
Should it be put under version control instead? (honest question)
There was a problem hiding this comment.
I haven’t encountered a case where it shouldn’t be. It helps catch mistakes in any case.
I love how much fixes this PR brings in! |
Hehe, seriously? |
:) Argh, it seems I was in the wrong git worktree when I pushed an update to the branch yesterday, and I accidentally included a merge to master that also contained some other work. Sorry. |
|
Hi, I thought I should give some feedback on my intentions for this PR. (I've been silent for far too long, sorry about that; difficulties finding enough time.) I'd like to first merge and push a few branches I've had boiling locally for some time (one of which was accidentally pushed here). Then I'd like to tend to this PR. I'm for including it, but I want
About the second point: got to check if one could use rebar3 profiles to achieve it somehow, eg First steps would anyway be to squash or amend all the various fixups and undo the accidental merge. Related is also that I have (only locally still) an improvement to gpb-eqc, to quickcheck flat oneofs. Will have to include that one here too. One reason I haven't had too much of a hurry on this is also that I locally have a Jenkins flow that runs the gpb-eqc checks before I push. Still, I agree of course that it'd be a good thing to have included out of the box. (I do not (yet) locally have checks with various versions of protobuf, only the 3.0.0, so #144 is a little important.) I'll write more when I start working on it again. |
|
These are valid points, and true I have been anxious for this PR to get merged :) but no rush really!
|
|
Iirc, I think what goes wrong with rebar2 is that proper isn't recognized as a dep and then the eunit tests will fail. (But memory fades quickly so I'm not 100% sure this was it.) I'd find it ok if PropEr-tests could be easily run with rebar3. Making it easy and optional to run them with rebar2 would not be high prio. But they mustn't fail with rebar2. Thanks for the pointer to rebar_proper, I'll be looking into it. |
|
I just created #151 as a continuation of this PR. I rebased it, but I don't think it is possible for me to rebase your PR, so I created a new. If it is ok with you, we close this PR and continue the discussion in the #151. Or just merge it if you're ok with how it currently looks (I'll squash the fixups first, of course) |
|
Included in 4.3.2, hence closing. |
This is a starting point for more thorough testing.
I intend on extending https://github.com/tomas-abrahamsson/gpb-eqc
with PropEr's Targeted testing properties.