protoc-gen-validate icon indicating copy to clipboard operation
protoc-gen-validate copied to clipboard

Published 20 hours ago • verified publisher icon bufbuild

go: check for NaN in lt, lte, gt, gte

Open torkelrogstad opened this issue 2 years ago • 11 comments

Closes #548

Don't know if this is supposed to have tests or not. Not really familiar with Bazel, so would need some pointers as to where to proceed on that.

torkelrogstad avatar Jan 20 '22 09:01 torkelrogstad

@rodaine would it be possible to get some eyes on this?

I believe the CI failures are not related. If so, I think this is something that fixes a bug quite a few people would stumble upon (I've done so, at least)

torkelrogstad avatar Jan 25 '22 16:01 torkelrogstad

Ping @Reflejo

torkelrogstad avatar Feb 10 '22 13:02 torkelrogstad

Ping @rmichela

:pray:

torkelrogstad avatar Mar 11 '22 17:03 torkelrogstad

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Apr 16 '22 16:04 stale[bot]

This is not stale, please remove the tag.

torkelrogstad avatar Apr 16 '22 17:04 torkelrogstad

Rebased.

torkelrogstad avatar Apr 16 '22 17:04 torkelrogstad

@htuch is it possible to get some eyes on this?

torkelrogstad avatar Apr 16 '22 17:04 torkelrogstad

I'm not an active maintainer, I think @rmichela @lizan @snowp might be better folks to take a look. FWIW it seems totally reasonable to me, but we would definitely want to see tests for this behavior added.

htuch avatar Apr 18 '22 01:04 htuch

Thanks for replying @htuch. How would I get started on tests? Do you for example do a golden file setup where the result from the templates are compared against a known good result?

torkelrogstad avatar Apr 18 '22 13:04 torkelrogstad

Yes, there are places for declaring test cases, e.g. for string const constraint:

  • https://github.com/envoyproxy/protoc-gen-validate/blob/main/tests/harness/cases/strings.proto#L8
  • https://github.com/envoyproxy/protoc-gen-validate/blob/dc51e59d2e463996ef24e45621cf61f95fa64b0f/tests/harness/executor/cases.go#L739

You should be able to fit this into the relevant float tests in these directories.

htuch avatar Apr 19 '22 04:04 htuch

relates to #85

elliotmjackson avatar Sep 16 '22 14:09 elliotmjackson

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Oct 04 '22 21:10 CLAassistant

Excited to see new maintainers here @ElliotMJackson. I rebased this on newest main branch, and added what I think are tests? Can't get them to run locally, let's see what happens on CI.

torkelrogstad avatar Oct 04 '22 21:10 torkelrogstad

We are excited to be here @torkelrogstad! I've kicked off CI 🤞 If you could help make our CLAassistant happy next time you're online that would be great.

For future reference, run them locally with a make ci or a safer space is in the container:

docker build -t protoc-gen-validate .
docker run --rm protoc-gen-validate ci

elliotmjackson avatar Oct 05 '22 17:10 elliotmjackson

Found some issues here I'm working on fixing, pushing up later

torkelrogstad avatar Oct 18 '22 06:10 torkelrogstad

closing due to inactivity

elliotmjackson avatar Mar 20 '23 19:03 elliotmjackson