Broken build in after merging pull request #17

[StefanKopieczek]

Pull request 55436d1091eb9277315b93032ee530776cec3e74 breaks the Gossip Gatekeeper build: http://54.68.45.135:8080/job/Gossip%20Gatekeeper/48/console

The failing test passes on my PC, but failed twice in a row on Jenkins.

[StefanKopieczek]

--- FAIL: TestSipUri (0.00 seconds)
    stringify_test.go:25: [FAIL] SIP URI with three parameters: Expected:             "sip:alice@wonderland.com;food=cake;something;drink=tea", Got:     "sip:alice@wonderland.com;food=cake;something;drink=tea"
    stringify_test.go:25: [FAIL] SIP URI with three headers: Expected:     "sip:alice@wonderland.com?CakeLocation="Tea Party"&Identity="Mad Hatter"&OtherHeader="Some value"", Got: "sip:alice@wonderland.com?CakeLocation="Tea Party"&Identity="Mad Hatter"&OtherHeader="Some value""
stringify_test.go:30: Passed 11/13 tests
FAIL
FAIL    github.com/stefankopieczek/gossip/base  0.002s

[StefanKopieczek]

... And another failure in http://54.68.45.135:8080/job/Gossip%20Gatekeeper/47/console with only one failed test this time:

--- FAIL: TestSipUri (0.00 seconds)
    stringify_test.go:25: [FAIL] SIP URI with three headers: Expected:     "sip:alice@wonderland.com?CakeLocation="Tea Party"&Identity="Mad Hatter"&OtherHeader="Some value"", Got: "sip:alice@wonderland.com?CakeLocation="Tea Party"&Identity="Mad Hatter"&OtherHeader="Some value""
    stringify_test.go:30: Passed 12/13 tests
FAIL
FAIL    github.com/stefankopieczek/gossip/base  0.002s

Since the failure isn't consistent, this feels like some kind of string internment bug.

[rynorris]

I don't understand. At that point it's just doing a direct string comparison on built-in strings. If they look the same in the output (which they do), the test should pass.

The only thing is that the test calls test.input.String() twice, rather than storing the value from the first time. So presumably SipUri.String() is somehow not deterministic, and the first call must've returned the wrong value.

[StefanKopieczek]

Yeah, I agree it's not internment.

Your analysis is one option. Another possibility is some kind of unicode comparison issue or non-printing character thing, although I don't see how that could happen.

I'm just fixing the build box at the moment, as I tried to repro and the test passed, so had to revert your commit in master. Dev now doesn't contain master, so I'm just fixing the build box to make sure we can't do non-ff merges – I'll fix up Dev to allow the merge later.

[rynorris]

Ok, got it. ParamsToString iterates over parameters using "for k, v := range params". params is a map, so iteration order is undefined. (and I believe intentionally randomised by the runtime to prevent you relying on it).

It's pure chance that the failed test output ended up matching in all 3 cases.

This seems like perhaps a bug. We probably shouldn't randomly re-order parameters if we want to be usable as a transparent proxy (even if it shouldn't cause any harm)

[StefanKopieczek]

Ah, of course.

[StefanKopieczek]

FIne, can you hold off pushing to Dev just a sec?

[rynorris]

I guess the fix is the same as we did for headers and have an ordering slice inside the Params type?

[StefanKopieczek]

OK, feel free to push a fix. I'll turn the gatekeeper back on tomorrow so that it doesn't re-merge the bad code between now and then.

[StefanKopieczek]

Yeah

[rynorris]

Urgh, but that's going to make tests so much worse to write. =S

[StefanKopieczek]

Fix the stringify tests for now, and raise an issue against the Params thing.

[StefanKopieczek]

I shouldn't have had to revert master and then turn off the build machine to get us back into a good state. I should give some thought to how to prevent this happening again.

Maybe just run the tests 50 times? :p

[StefanKopieczek]

Actually, that's not a terrible idea, even if it is a bit of a hack. It gives us at least some defence against tests which only sometimes fail. I've made the change.

[rynorris]

No way to fix other than commenting out those tests for now. I'll do that now.

I guess with a builder pattern it wouldn't be so bad. ie:

Params{"food": String{"cake"}, "something": NoString{}}

becomes

NewParams().Add("food", String{"cake"}).Add("something", NoString{})

But changing all the parser tests is gonna be a huge pain.

[StefanKopieczek]

Agree that looks like the right fix. Also agree we can't fix the tests, so commenting them out is probably fine. I'm happy to own that fix and change the tests if you like. Let's track with this issue. Pass to me unless you fancy doing it (you're more than welcome to, of course).

Sorry for the slightly disjointed replies, I need to get some sleep.

[rynorris]

I'll comment out the tests. You reverted the whole merge? So I should do it back on the MaybeString branch?

[StefanKopieczek]

Yes, do it on MaybeString and remerge (you don't need to submit a pull request).

I reverted the entire merge because I needed to revert it in master, and Gatekeeper now (sensibly) doesn't allow non-ff merges from Dev to Master so I had to revert it in Dev.

This shouldn't happen again if we make Gatekeeper re-run the tests a bunch of times to catch random errors, which I'm not doing.

[StefanKopieczek]

Thanks, the build's working fine now (with the commented out tests).

[StefanKopieczek]

I think I'll close this and raise a new issue with a more appropriate title.