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

code generator is missing unit tests #8

Closed
pkositsyn opened this issue Aug 20, 2020 · 3 comments
Closed

code generator is missing unit tests #8

pkositsyn opened this issue Aug 20, 2020 · 3 comments

Comments

@pkositsyn
Copy link

pkositsyn commented Aug 20, 2020

  1. If you look at RecordBatch type, there is a crc field which must be used to decode the following data according to https://kafka.apache.org/documentation.html#messageformat. It is not enough just to decode fields as they are
  2. In my opinion, this repo lacks tests, though I understand that there is a ton of generated code, which needs to be tested
@twmb
Copy link
Owner

twmb commented Aug 24, 2020

re (1), that's true, but the intent of the kmsg package itself is just to decode, not validate. I could add a function in the interface.go file (non-generated file) to do some message validation in the kmsg package itself. I haven't because I historically have not intended that package to be the primary way to decode records: I figure if you're decoding records, just use the kgo package. However, your issue did remind me that I do not have crc validation on decode in the kgo package itself. I'll go ahead and add that now (this was less important to me during development since I intend to rely on TLS during actual usage).

re (2), that's also mildly true. The kgo package includes a three length ETL chain for groups and transactions using this chaining code. All group algorithms are tested in that chaining code as well, and there are other random dedicated unit tests. I do agree though that the generator should have some unit tests just to avoid regressions (albeit the generator is becoming more and more static). I'll look into adding some tests there.

My takeaway from this issue is that it has two parts,

  • add crc validation to decoding in source.go
  • add unit tests to generation in the generate package

@twmb twmb changed the title Some issues missing CRC validation & code generator is missing unit tests Aug 24, 2020
twmb added a commit that referenced this issue Aug 24, 2020
As pointed out in #8, I had no crc validation on decode.
The most obvious place to add that validation (and length validation) is
in kmsg, which also makes this validation more broadly applicable to
those that do not want to use the kgo package.

This has been tested with the chaining tests, and v0 / v1 message sets
have been tested with kcl.
twmb added a commit that referenced this issue Aug 24, 2020
As pointed out in #8, I had no crc validation on decode.
The most obvious place to add that validation (and length validation) is
in kmsg, which also makes this validation more broadly applicable to
those that do not want to use the kgo package.

This has been tested with the chaining tests, and v0 / v1 message sets
have been tested with kcl.
@twmb
Copy link
Owner

twmb commented Aug 24, 2020

The crc validation was fixed with 5982615, which necessitated a new tag (v0.4.7). For record batches, this was tested with the chaining test in the kgo package itself. For v0/v1 message tests, this was tested with kcl by consuming with --as-version 0.9.0 and --as-version 0.10.0.

The unit tests for code generation will not be api breaking and can come later.

@twmb twmb changed the title missing CRC validation & code generator is missing unit tests ~missing CRC validation~ & code generator is missing unit tests Aug 24, 2020
@twmb twmb changed the title ~missing CRC validation~ & code generator is missing unit tests code generator is missing unit tests Aug 24, 2020
@twmb
Copy link
Owner

twmb commented Apr 13, 2021

I'm not too interested in writing unit tests for the generator at this point. The generated code has been tested extensively externally, on top of the internal integration tests. I've changed how code is generated multiple times at this point and am planning on other changes shortly. In light of this, unit tests on generation would mostly just be tests to freeze behavior, which I do not want, and would not actually be that useful.

@twmb twmb closed this as completed Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants