cburrows
commented
7 years ago Looks a lot like issues decoding Strings. utf8ToString() seems suspicious.
Closed cburrows closed 7 years ago
cburrows
commented
7 years ago Looks a lot like issues decoding Strings. utf8ToString() seems suspicious.
tbkka
commented
7 years ago I see two groups of issues:
Test_AllTypes.swift:608: error: Test_AllTypes.testEncoding_optionalString : XCTAssertTrue failed - Encode/decode cycle should generate equal object: SwiftProtobufTests.ProtobufUnittest_TestAllTypes:
optional_string: ""
!= SwiftProtobufTests.ProtobufUnittest_TestAllTypes:
optional_string: "\000\001\002\003\004\005\006\007\b\t\n"There are a series of tests that include \000 null bytes in them. It looks like these strings are getting truncated at the zero byte.
Test_AllTypes.swift:1310: error: Test_AllTypes.testEncoding_repeatedString : failed - Swift decode should have failed: [226, 2, 1, 193]This is one of several tests that include broken UTF8. Apparently, such broken UTF8 is not getting rejected as it should.
tbkka
commented
7 years ago I'm still seeing six failures after #372:
Test_JSON.swift:499: error: Test_JSON.testSingleString : XCTAssertTrue failed - Object should not be equal to empty object
Test_JSON.swift:505: error: Test_JSON.testSingleString : XCTAssertTrue failed - Object should not be equal to empty object
Test_JSON.swift:508: error: Test_JSON.testSingleString : XCTAssertTrue failed - Object should not be equal to empty object
Test_TextFormat_proto3.swift:453: error: Test_TextFormat_proto3.testEncoding_singleString : XCTAssertTrue failed - Object should not be equal to empty object
Test_TextFormat_proto3.swift:461: error: Test_TextFormat_proto3.testEncoding_singleString : XCTAssertTrue failed - Object should not be equal to empty object
Test_TextFormat_proto3.swift:465: error: Test_TextFormat_proto3.testEncoding_singleString : XCTAssertTrue failed - Object should not be equal to empty objectThese all seem to be symptoms of https://bugs.swift.org/browse/SR-4218
Working around this is tough: the only approach I've come up with is to have the generated equality test invoke a function in the SwiftProtobuf library where we can fudge the behavior on a per-platform basis. That could get very complicated.
thomasvl
commented
7 years ago Yea, that sounds like a little too much hacking to me.
Maybe we plan on saying the 1.0 milestone is good for any appleOS, but prerelease for linux due to Swift and Foundation not being fully up to spec there?
cburrows
commented
7 years ago I see exactly these six problems too and discovered https://bugs.swift.org/browse/SR-4218 myself (with "\u{00}" and ""). I don't think we should hack it ... but can we mark these as known failures on Linux?
tbkka
commented
7 years ago I'm happy to ship 1.0 with "known issues on Linux". Obviously, I'd like there to be as few such issues as possible.
cburrows
commented
7 years ago Right, I meant make the tests as known-to-fail. It's possible the "object should not be equal to empty object" failures are actually pure test failures against correctly operating code. The problem with those failures is the assert itself, which is calling the string equality test.
String equality that does not behave correctly seems ... exceptionally bad to me. And not something we could reasonably work around.
Anyway, I see 13 failures now ...
/root/swift-protobuf/Tests/SwiftProtobufTests/Test_AllTypes.swift:632: error: Test_AllTypes.testEncoding_optionalString : failed - Swift decode should have failed: [114, 1, 193]
/root/swift-protobuf/Tests/SwiftProtobufTests/Test_AllTypes.swift:632: error: Test_AllTypes.testEncoding_optionalString : failed - Swift decode should have failed: [114, 1, 193]
/root/swift-protobuf/Tests/SwiftProtobufTests/Test_AllTypes.swift:1317: error: Test_AllTypes.testEncoding_repeatedString : failed - Swift decode should have failed: [226, 2, 1, 193]
/root/swift-protobuf/Tests/SwiftProtobufTests/Test_AllTypes.swift:2055: error: Test_AllTypes.testEncoding_oneofString : failed - Swift decode should have failed: [138, 7, 1, 192]
/root/swift-protobuf/Tests/SwiftProtobufTests/Test_AllTypes_Proto3.swift:597: error: Test_AllTypes_Proto3.testEncoding_singleString : failed - Swift decode should have failed: [114, 1, 193]
/root/swift-protobuf/Tests/SwiftProtobufTests/Test_AllTypes_Proto3.swift:1111: error: Test_AllTypes_Proto3.testEncoding_repeatedString : failed - Swift decode should have failed: [226, 2, 1, 193]
/root/swift-protobuf/Tests/SwiftProtobufTests/Test_AllTypes_Proto3.swift:1294: error: Test_AllTypes_Proto3.testEncoding_oneofString : failed - Swift decode should have failed: [138, 7, 1, 192]
/root/swift-protobuf/Tests/SwiftProtobufTests/Test_JSON.swift:499: error: Test_JSON.testSingleString : XCTAssertTrue failed - Object should not be equal to empty object
/root/swift-protobuf/Tests/SwiftProtobufTests/Test_JSON.swift:505: error: Test_JSON.testSingleString : XCTAssertTrue failed - Object should not be equal to empty object
/root/swift-protobuf/Tests/SwiftProtobufTests/Test_JSON.swift:508: error: Test_JSON.testSingleString : XCTAssertTrue failed - Object should not be equal to empty object
/root/swift-protobuf/Tests/SwiftProtobufTests/Test_TextFormat_proto3.swift:453: error: Test_TextFormat_proto3.testEncoding_singleString : XCTAssertTrue failed - Object should not be equal to empty object
/root/swift-protobuf/Tests/SwiftProtobufTests/Test_TextFormat_proto3.swift:461: error: Test_TextFormat_proto3.testEncoding_singleString : XCTAssertTrue failed - Object should not be equal to empty object
/root/swift-protobuf/Tests/SwiftProtobufTests/Test_TextFormat_proto3.swift:465: error: Test_TextFormat_proto3.testEncoding_singleString : XCTAssertTrue failed - Object should not be equal to empty object
/root/swift-protobuf/Tests/SwiftProtobufTests/Test_TextFormat_proto3.swift:567: error: Test_TextFormat_proto3.testEncoding_singleString_UTF8 : failed - Swift decode should have failed: single_string: "\342\230" "\236"The other failures you list all have the form "decode should have failed" and are all exercising decoding an invalid UTF-8 string. (A valid UTF-8 string can never end with 192 or 193, and the final text format example breaks a three-byte UTF-8 sequence across two input strings, which is not legal.)
I do not see those failing with the March 8 or March 9 snapshots on Ubuntu 16.10, so I presume they've been fixed in Swift master since the version you're using.
cburrows
commented
7 years ago I only saw 6 failures earlier, and after a git clean -x -d -f I saw 13, having changed nothing. Is this the swift you're using?
root@e4f0f585f609:~# swift --version
Swift version 3.0.2 (swift-3.0.2-RELEASE)
Target: x86_64-unknown-linux-gnuOh--I see it's not. You're running dev snapshots. Is the plan at 1.0 not to support 3.0.2?
tbkka
commented
7 years ago As I mentioned earlier, I am willing to ship 1.0 with "known issues on Linux" if those issues are really Swift bugs that we cannot reasonably work around.
That these failures don't appear on later dev snapshots suggests they really are Swift bugs.
The repercussions of these particular failures are modest: It means that we would not issue errors when deserializing input that contained malformed UTF-8.
So the question becomes: What would be required to work around them? If it's simple enough, we should do so, but I've not had a chance to dig into them yet.
tbkka
commented
7 years ago Right, I meant make the tests as known-to-fail.
Does XCTest have such a notion? Can we mark these tests only on Linux? Does XCTest complain if a "known-to-fail" test actually succeeds?
thomasvl
commented
7 years ago As far as I know, XCTest doesn't have a "known to fail".
Our best bet is like to disable the tests on linux (#if), and add a linux specific test that uses the Swift version checks to XCFail() when run under a new version of swift we having tested so we get a reminder to revisit the disabled tests.
Maybe something like:
fun test_Foo() {
#if os(Linux)
#if swift(>=3.0)
XCTFail("Check if this test now passes on Linux")
#endif
#else
// The real test
#endif
}PRs #438 and #439 fixed some additional minor issues that crept in.
tbkka
commented
7 years ago PR #441 implements a slightly simpler approach along the same lines.
tbkka
commented
7 years ago Fixed via #441.
For instance, using ibmcom/swift-ubuntu, there are 24 failures on master right now. I have not investigated them at all.