Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

towards better module testing #3397

Merged

Conversation

DanielaE
Copy link
Contributor

  • use the standard test-main.cc component instead of injected test infrastructure sources
  • undo now obsolete commit 00235d8a from July 2021
  • test current API: remove obsolete tests, add tests for new API functions

@DanielaE DanielaE force-pushed the feature/towards-better-module-testing branch from 053cb16 to 978d365 Compare April 23, 2023 12:12
@vitaut
Copy link
Contributor

vitaut commented Apr 26, 2023

Now that module support is getting better, I think we should kill a separate module test and make normal tests compatible with modules. This will avoid duplication and improve coverage.

@DanielaE
Copy link
Contributor Author

I agree. All the tests for internal, non-exported entities need to be excluded from the test suites when the API is brought in through imports rather than includes. As far as I can see, this requires

  • the ability to build a {fmt} module, preferably also consumable by users of the library
  • for all compilers that have reasonable support for C++ modules (i.e. clang and msvc these days)
  • with all the necessary workarounds applied (this affects both clang and msvc)
  • subjected to all test suites on CI test runners.

For the time being I have this working only in the module testing branch of my fork of {fmt} that focuses on the API surface, the development of work arounds, and the deployment of CI. This builds on the existing, old test approach and the related infrastructure in CMakeTests.txt.

@vitaut
Copy link
Contributor

vitaut commented Apr 29, 2023

We don't need to modularize all tests at once. Even modularizing core-test alone would be better than the current state.

# include "fmt/os.h"
#endif // FMG_MODULE_TEST

#include "fmt/os.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With all compilers having converged to the 'strong module ownership' model we no longer need to fold module fmt into the test machinery for module tests. The module has all its symbols attached to the module (e.g. void foo() becomes void foo@fmt()), and the test machinery can have the same definitions attached to the global module within the same program.

Therefore, the test-main library can be reused as with all other tests. And all stuff conditionally compiled by FMT_MODULE_TEST can kicked out again. Both Clang and MSVC are fine with it. Strong ownership rulez!

Comment on lines 520 to 531
TEST(module_test, std_types) {
EXPECT_EQ(R"("42")", fmt::format("{}", std::filesystem::path("42")));
EXPECT_EQ(R"(optional("42"))",
fmt::format("{}", std::optional<std::string>("42")));
EXPECT_EQ("none", fmt::format("{}", std::optional<std::string>()));
EXPECT_EQ("variant(42)",
fmt::format("{}", std::variant<int, std::string>(42)));
EXPECT_EQ("monostate", fmt::format("{}", std::monostate()));
EXPECT_EQ("system:0", fmt::format("{}", std::error_code()));
EXPECT_EQ("0", fmt::format("{}", std::thread::id()));
EXPECT_EQ("42", fmt::format("{}", std::runtime_error("42")));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should stop adding new test cases to module-test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can drop this for your repo and keep it in mine. I want tests now.

 * use the standard `test-main.cc` component instead of injected test infrastructure sources
 * undo now obsolete commit `00235d8a` from July 2021
 * Clang cannot import user-defined literals as it seems -> disable test
 * Clang emits duplicate, non-mergeable copies of `detail::buffer`'s vtable, causing linker errors -> disable test
@DanielaE DanielaE force-pushed the feature/towards-better-module-testing branch from 978d365 to 409ed46 Compare May 2, 2023 15:48
@vitaut vitaut merged commit d7a8e50 into fmtlib:master May 3, 2023
@vitaut
Copy link
Contributor

vitaut commented May 3, 2023

Thank you!

@DanielaE DanielaE deleted the feature/towards-better-module-testing branch May 3, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants