Skip to content

Add a conformance test suite#251

Open
Flaque wants to merge 2 commits intocel-rust:masterfrom
sfcompute:conformance-suite
Open

Add a conformance test suite#251
Flaque wants to merge 2 commits intocel-rust:masterfrom
sfcompute:conformance-suite

Conversation

@Flaque
Copy link
Copy Markdown

@Flaque Flaque commented Jan 9, 2026

This PR follows up on #250 by just adding the conformance test suite harness.

@alexsnaps
Copy link
Copy Markdown
Member

This currently fails with:

Could not find protoc

Now, I'd be in the camp of adding the output .rs files from the .protos to the repo. My understanding is that we don't expect these to change by themselves and that as such the output should also not change. This would save time and resources in the CI. wdyt?

@howardjohn
Copy link
Copy Markdown
Contributor

you can also use protox which is native rust and doesn't depend on protoc fwiw.

@alexsnaps
Copy link
Copy Markdown
Member

you can also use protox which is native rust and doesn't depend on protoc fwiw.

Right, I'm mostly concerned about resources wasted here. Arguably it won't be too much an issue for some time, but on project with more collaborators and as such PRs (on a whole lot more repos too tbh), but which has a "paid for account", we get throttled quite a bit. So if there is no need to run any .proto compilation (whatever the mean), I'd opt for the lazy approach. If only, that'd probably shorten the feedback loop on CI running to an end as well, which is always desirable I think.

@alexsnaps
Copy link
Copy Markdown
Member

alexsnaps commented Jan 14, 2026

you can also use protox which is native rust and doesn't depend on protoc fwiw.

Just to be clear tho, I think the build.rs should be there, with matching cargo:rerun-if-changed, for automatic generation when needed. I just wouldn't do it on every builds, as that's not needed and speeds builds (also doesn't require further installs for "newcomers" - tho, if @howardjohn 's proposal mitigates that and protox being up to our needs, I'm totally fine using that)

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

Successfully merging this pull request may close these issues.

3 participants