Page MenuHomeWildfire Games

tests: Fix cppformat failures on osx (locale-related)
ClosedPublic

Authored by Krinkle on Jun 15 2019, 7:37 PM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22378: Fix cppformat failures on MacOS tests.
Summary
TestCppformat::test_basic:
test_cppformat.h:36: Expected …, found ("0,500000" != 0.500000)
test_cppformat.h:37: Expected …, found ("0,1" != 0.1)

This test varies behaviour by locale, which caused the test to fail on osx by default.
This probably failed on Linux as well, depending on the locale of the host. The
current test fixture seems to expect US-notation, so I've fixed it to that locale.

Test Plan
  • In ./build/workspaces/gcc, run make.
  • Run ./binaries/system/test.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7944
Build 12929: arc lint + arc unit

Event Timeline

Krinkle created this revision.Jun 15 2019, 7:37 PM
Krinkle edited the test plan for this revision. (Show Details)Jun 15 2019, 7:39 PM
wraitii added a subscriber: wraitii.

Assigning to myself as I can reproduce the issue.

source/ps/tests/test_cppformat.h
29

I think you can just do char* old = setlocale(LC_ALL, "en_US.UTF-8"); but I'll check.

Krinkle added a reviewer: Restricted Owners Package.Jun 15 2019, 7:43 PM
vladislavbelov added inline comments.
source/ps/tests/test_CStr.h
135

Wasn't it the idea? That 3bogus should be interpreted as 3?

Krinkle updated this revision to Diff 8489.Jun 15 2019, 7:52 PM

Simplify setlocale() call.

Harbormaster completed remote builds in B7947: Diff 8489.
Krinkle added inline comments.Jun 15 2019, 7:52 PM
source/ps/tests/test_cppformat.h
29

I copied this from the CStr test file. But yeah, looks like the intermediate step isn't needed.

Krinkle added inline comments.Jun 15 2019, 7:57 PM
source/ps/tests/test_CStr.h
135

Yeah, I understand changing the input is an unusual way to fix a test (see commit msg).

I'm not sure how this ties into the rest of 0AD and where this particular tolerance is used/wanted. Does it accept this kind of value as user input somewhere in the game?

Presumably if that is the case then there is likely a bug in the OSX release, which would be a nice first bug to reproduce and fix for me in that case. It doesn't seem related to locales though, so that kind of fix might require more changes e.g. to the library version or something like that. If so, I'll leave this part out of the patch as I'm not experienced enough with CPP to handle that right now.

Krinkle marked an inline comment as done.Jun 15 2019, 7:58 PM
Krinkle marked an inline comment as not done.
vladislavbelov added inline comments.Jun 15 2019, 8:09 PM
source/ps/tests/test_CStr.h
135

I'd like to know a reason, why it's parsed differently. Something probably related:

https://forum.qt.io/topic/103340/mac-stringstream-returns-wrong-output/18

Krinkle updated this revision to Diff 8490.Jun 15 2019, 8:47 PM
Krinkle retitled this revision from tests: Fix CStr and cppformat failures on osx (locale-related) to tests: Fix cppformat failures on osx (locale-related).
Krinkle edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B7948: Diff 8490.
Krinkle added inline comments.Jun 15 2019, 8:48 PM
source/ps/tests/test_CStr.h
135

Okay, I've reduced my patch to the other test issue for now. It's not an area I can help with right now.

wraitii accepted this revision.Jun 16 2019, 11:27 AM

Fixes my issue and seems correct to update the locale with regards to blame of this test.

On the CStr issue - it appears OSX is doing something weird - same issue as the guy there indeed it seems, good find @vladislavbelov.

If I run:

#include <sstream>

int main()
{
	// 97 to 122 or 65 to 90
	for (char i = 65; i <= 90; i++)
	{
		std::string test = "2", test_non_cap = "2";
		test += i;
		test_non_cap += i + 32;
		std::stringstream stream(test), stream_non_cap(test_non_cap);
		float out, out_non_cap;
		stream >> out;
		stream_non_cap >> out_non_cap;
		printf("%s: %f -- %s: %f\n", test.c_str(), out, test_non_cap.c_str(), out_non_cap);
	}
	return 0;
}

It outputs:

2A: 0.000000 -- 2a: 0.000000
2B: 0.000000 -- 2b: 0.000000
2C: 0.000000 -- 2c: 0.000000
2D: 0.000000 -- 2d: 0.000000
2E: 0.000000 -- 2e: 0.000000
2F: 0.000000 -- 2f: 0.000000
2G: 2.000000 -- 2g: 2.000000
2H: 2.000000 -- 2h: 2.000000
2I: 0.000000 -- 2i: 0.000000
2J: 2.000000 -- 2j: 2.000000
2K: 2.000000 -- 2k: 2.000000
2L: 2.000000 -- 2l: 2.000000
2M: 2.000000 -- 2m: 2.000000
2N: 0.000000 -- 2n: 0.000000
2O: 2.000000 -- 2o: 2.000000
2P: 0.000000 -- 2p: 0.000000
2Q: 2.000000 -- 2q: 2.000000
2R: 2.000000 -- 2r: 2.000000
2S: 2.000000 -- 2s: 2.000000
2T: 2.000000 -- 2t: 2.000000
2U: 2.000000 -- 2u: 2.000000
2V: 2.000000 -- 2v: 2.000000
2W: 2.000000 -- 2w: 2.000000
2X: 0.000000 -- 2x: 0.000000
2Y: 2.000000 -- 2y: 2.000000
2Z: 2.000000 -- 2z: 2.000000

which is pretty weird.

This revision is now accepted and ready to land.Jun 16 2019, 11:27 AM
This revision was automatically updated to reflect the committed changes.