Update conformance tests to protocolbuffers/protobuf v34.0#1391
Conversation
Upgrade the conformance test runner and its bundled protos from v33.2.0 to v34.0.0 across the Node, Bun, and Deno conformance packages.
emcfarlane
force-pushed
the
ed/protocConformV34
branch
from
7c3fbc7 to
809b216
Compare
srikrsna-buf
left a comment
srikrsna-buf
left a comment
There was a problem hiding this comment.
Instead of failing Recommended.Editions.ProtobufInput.RejectInvalidUtf8.String.Extension can we update the conformance script to read set extensions. We can get this from the unknown fields and check the registry for each unknown tag.
|
@srikrsna-buf thanks! Updated so each conformance test calls |
Good call. We want the confidence that the implementation is conformant. Because we parse extensions lazily, we have to take an extra step to surface errors, but we do want to surface and validate the errors. |
timostamm
left a comment
timostamm
left a comment
There was a problem hiding this comment.
LGTM - just one comment suggestion to clarify why we're calling getExtension in conformance.ts
| // fromBinary stores extensions as unknown fields. Realize any whose | ||
| // descriptor is known to the registry so that per-field validation | ||
| // (e.g. UTF-8 checks for string extensions) fires at parse time. |
There was a problem hiding this comment.
| // fromBinary stores extensions as unknown fields. Realize any whose | |
| // descriptor is known to the registry so that per-field validation | |
| // (e.g. UTF-8 checks for string extensions) fires at parse time. | |
| // We parse extensions lazily. To exercise the conformance tests for | |
| // UTF-8 validation, we have to take this extra step: In addition to parsing | |
| // the message, we also parse all extensions. |
(same for deno/conformance and packages/conformance)
3 participants
This PR updates the conformance testing to protocolbuffers/protobuf v34.0. Follows on from #1385.
Extensions are now handled in conformance to assert validation. We call
getExtensionon all unknowns. This passes the recommended testcaseRecommended.Editions.ProtobufInput.RejectInvalidUtf8.String.Extension. Other issues were already addressed in #1388, #1387 and #1386.Closes #1363