More module tests #2309

DanielaE · 2021-05-23T17:06:33Z

Most of the API of core.h, format.h, and args.h

Just narrow strings. Tests for wide strings will come later.

vitaut

Thanks for the PR. Having a module test seems like a good idea but I am concerned about duplication. I think we don't need to replicate so many test cases here, it's enough to exercise every user-facing public function like format or format_to once. In particular, we shouldn't be testing multiple built-in argument types because they all go through the same path.

Also there are some json files that I don't think should be here.

DanielaE · 2021-05-25T13:42:24Z

Right, these two additional files sneaked in by accident. Thanks for pointing this out.

Regarding duplication: it's not just the name like e.g. format but all of the overloads, and as these are templates which may cause different specializations within the library, as many of them should be instantiated to check not only visibility but also reachability. This aspect is a non-issue in traditional libraries, because everything visible is also reachable, and there are possibly more declarations and semantic properties reachable than visible. In the modules world things are much different: visibility and name-lookup in instantiations must be put to a test. I've already found a few places by these tests where lookup is an issue that need to be audited and addressed.

If you happen to have sections to point me to which concern you I will look into them and remove superfluous duplicates. I'd love to have a tool to aide me but I'm not aware of one.

vitaut · 2021-05-26T14:17:01Z

test/module-test.cc

+  EXPECT_EQ("42", fmt::format("{}", 42));
+  EXPECT_EQ("42", fmt::format("{:}", 42u));
+  EXPECT_EQ("+42", fmt::format("{:+d}", short{42}));
+  EXPECT_EQ("42", fmt::format("{:d}", unsigned short{42u}));
+  EXPECT_EQ("-42", fmt::format("{0}", -42L));
+  EXPECT_EQ("2a", fmt::format("{0:x}", 42uL));
+  EXPECT_EQ("***42", fmt::format("{:*>5}", 42LL));
+  EXPECT_EQ("0X2A", fmt::format("{:#X}", 42uLL));
+  EXPECT_EQ("===42.000000===", fmt::format("{:=^15f}", 42.0));
+  EXPECT_EQ("00000042.1", fmt::format("{:<010.5G}", 42.1f));
+  EXPECT_EQ(" +4.21235e+11", fmt::format("{:+{}.{}e}", 42.1234567e10L, 13, 5));
+  EXPECT_EQ("-0x1.5000000000000p+5", fmt::format("{: a}", -42.0));
+  constexpr auto nan = std::numeric_limits<double>::quiet_NaN();
+  EXPECT_EQ("-nan-", fmt::format("{:-^5}", nan));
+  constexpr auto inf = std::numeric_limits<double>::infinity();
+  EXPECT_EQ("-INF-", fmt::format("{:-^5F}", inf));
+  EXPECT_EQ("-INF ", fmt::format("{: 5G}", -inf));
+  EXPECT_EQ("42", fmt::format("{:s}", "42"));
+  EXPECT_EQ("a", fmt::format("{:c}", 'a'));
+  EXPECT_EQ("a", fmt::format("{:c}", unsigned char{'a'}));
+  EXPECT_EQ("a", fmt::format("{:c}", signed char{'a'}));
+  EXPECT_EQ("false", fmt::format("{}", false));
+  EXPECT_EQ("0B1", fmt::format("{:#B}", true));
+  EXPECT_EQ("0x2a", fmt::format("{:p}", (void *)42));
+  EXPECT_EQ("1234", fmt::format("{:L}", 1234));


For example here we don't need to test all argument types. Since they all go through the same code path, it is very unlikely that one of them works and another doesn't. I suggest picking one type (e.g. int) and testing public methods with it. In the unlikely case that we find some problem in the future, we can add a regression test.

test/module-test.cc

DanielaE · 2021-05-27T15:39:01Z

Your suggestions are in. This is also updated to msvc 16.11-pre1

vitaut

Your suggestions are in. This is also updated to msvc 16.11-pre1

Thank you. A few more comments, otherwise looks great.

vitaut · 2021-05-28T14:00:52Z

test/module-test.cc

+  EXPECT_FALSE(no_args.get(1));
+
+  using ctx = fmt::format_context;
+  fmt::basic_format_args<ctx> args = fmt::make_format_args<ctx>(42);


ctx can be dropped here because it is the default. Same in multiple places below.

vitaut · 2021-05-28T14:06:32Z

test/module-test.cc

+}
+
+TEST(module_test, ptr) {
+  auto p = (int*)42;


Please use more narrow bit_cast or reinterpret_cast instead of a C-style cast.

vitaut · 2021-05-28T14:08:25Z

test/module-test.cc

+}
+
+TEST(module_test, weekday) {
+  const auto de = std::locale("de_DE");


This may throw if the locale is not available.

Yeah ...
Luckily we don't care here.

Why don't we care? Since this is a non-functional test I suggest using the std::locale::classic() which is always available.

We don't care for the same reason why you suggest using the classic locale: it just has to look up the correct overloads. If you prefer std::locale::classic() then I'll change it accordingly.

it just has to look up the correct overloads.

It would enough if we didn't actually run this code, only compile. But my understanding is that we eventually want to run this code.

If you prefer std::locale::classic() then I'll change it accordingly.

Please do.

test/module-test.cc

vitaut · 2021-05-28T14:14:32Z

test/module-test.cc

+  static_assert(fmt::is_formattable<disabled_formatter_convertible>::value, "");
+}
+
+struct convertible_to_int {


Everything starting from here to the end of the file falls into the category of "functional testing", please remove. It is very unlikely that this functionality behaves differently between modular and non-modular build because it goes through the same argument handling paths as everything else.

I've condensed it as much as possible to name-lookup.

core.h, format.h, args.h, chrono.h, color.h, printf.h, os.h

vitaut · 2021-05-28T22:49:44Z

Thank you!

DanielaE force-pushed the feature/add-module-test branch 2 times, most recently from 1a2b2d1 to d5203d3 Compare May 25, 2021 09:36

vitaut requested changes May 25, 2021

View reviewed changes

vitaut reviewed May 26, 2021

View reviewed changes

test/module-test.cc Outdated Show resolved Hide resolved

DanielaE force-pushed the feature/add-module-test branch from d5203d3 to 700551f Compare May 27, 2021 12:52

DanielaE force-pushed the feature/add-module-test branch from 700551f to d8e200d Compare May 28, 2021 11:26

vitaut requested changes May 28, 2021

View reviewed changes

DanielaE force-pushed the feature/add-module-test branch from d8e200d to 865994b Compare May 28, 2021 15:45

More module tests

576f0b7

core.h, format.h, args.h, chrono.h, color.h, printf.h, os.h

DanielaE force-pushed the feature/add-module-test branch from 865994b to 576f0b7 Compare May 28, 2021 18:09

vitaut merged commit 272b0f3 into fmtlib:master May 28, 2021

DanielaE deleted the feature/add-module-test branch May 29, 2021 04:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More module tests #2309

More module tests #2309

DanielaE commented May 23, 2021

vitaut left a comment

DanielaE commented May 25, 2021

vitaut May 26, 2021

DanielaE commented May 27, 2021

vitaut left a comment

vitaut May 28, 2021

vitaut May 28, 2021

vitaut May 28, 2021

DanielaE May 28, 2021

vitaut May 28, 2021

DanielaE May 28, 2021

vitaut May 28, 2021

vitaut May 28, 2021

DanielaE May 28, 2021

vitaut commented May 28, 2021

More module tests #2309

More module tests #2309

Conversation

DanielaE commented May 23, 2021

vitaut left a comment

Choose a reason for hiding this comment

DanielaE commented May 25, 2021

Choose a reason for hiding this comment

DanielaE commented May 27, 2021

vitaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaut commented May 28, 2021