Update to protovalidate 0.10.3 #73
Conversation
|
Not ready for review yet. Some failing tests. In addition, the conformance runner sporadically segfaults with no discernible pattern. This happens locally and in CI. When the conformance tests actually run, they all pass with the current expected failure list. |
| @@ -201,7 +201,8 @@ TEST(ValidatorTest, ValidateBadURIRefFailure) { | |||
| ASSERT_TRUE(violations_or.ok()) << violations_or.status(); | |||
| EXPECT_EQ(violations_or.value().violations_size(), 1); | |||
| EXPECT_EQ(violations_or.value().violations(0).proto().constraint_id(), "string.uri_ref"); | |||
| EXPECT_EQ(violations_or.value().violations(0).proto().message(), "value must be a valid URI"); | |||
| EXPECT_EQ( | |||
There was a problem hiding this comment.
Verified my local clang-formatter is using the repo's clang-format rules.
| @@ -201,7 +201,8 @@ TEST(ValidatorTest, ValidateBadURIRefFailure) { | |||
| ASSERT_TRUE(violations_or.ok()) << violations_or.status(); | |||
| EXPECT_EQ(violations_or.value().violations_size(), 1); | |||
| EXPECT_EQ(violations_or.value().violations(0).proto().constraint_id(), "string.uri_ref"); | |||
| EXPECT_EQ(violations_or.value().violations(0).proto().message(), "value must be a valid URI"); | |||
| EXPECT_EQ( | |||
| violations_or.value().violations(0).proto().message(), "value must be a valid URI Reference"); | |||
There was a problem hiding this comment.
This was the actual test fix. No idea how this was ever passing.
| // If hostSplit has a size greater than 1, then a '/' appeared in the string. Set the rest | ||
| // to remainder so we can parse any query string. | ||
| if (hostSplit.size() > 1) { | ||
| remainder = hostSplit[1]; | ||
| } |
There was a problem hiding this comment.
This is the segfault fix. The rest are just formatting changes.
|
Update: it seems the difference may be related to Mac v. Linux envs. The current Will try to explore why that is, but in the meantime, I've disabled Update 2: the difference appears to be related to the usage of inet_pton(AF_INET6, str.data(), &result)It seems to return inconsistent results on Mac vs. Linux when validating tests involving zone IDs. I did a bit of searching to see if this is documented anywhere, but couldn't find anything. But, again, unless there's an obvious/easy fix, these validation methods are going to be rewritten, so not worth a big effort to fix. Worth noting though that protovalidate-cc may currently validate differently on Linux vs. Mac in some cases for ipv6 addresses. |
|
Updated to use two different files for Mac v. Linux. |
|
Thanks for debugging the issue. It feels obvious in retrospect, but I honestly was not getting very far when I looked myself. |
This includes changes to the conformance tests and conformance runner in v0.10.2 and v0.10.3.
For now, we're skipping 165 failing test cases:
NOTE: There are inconsistencies around some conformance tests (see
expected_failures.yamland search forFIXME). The current configuration in this PR passes CI with Bazel, but this will fail when running with CMake and running locally. There is an issue with how Bazel is reading these tests and it seems a bit of a waste to focus too much time since these tests are going to be fixed in the future anyway.TL;DR - The failing unit tests are because conformance tests are failing via CMake. However, the same
expected_failuresfile works for Bazel.