Skip to content

Re-Enable fmt module tests, quarantine failing code#4702

Draft
MathewBensonCode wants to merge 1 commit intofmtlib:masterfrom
MathewBensonCode:testmodules
Draft

Re-Enable fmt module tests, quarantine failing code#4702
MathewBensonCode wants to merge 1 commit intofmtlib:masterfrom
MathewBensonCode:testmodules

Conversation

@MathewBensonCode
Copy link
Contributor

  • Refactor test/CMakeLists.txt to enable testing for modules

  • Refactored tests related to fmt::format_args that now requires lvalue
    references as opposed to direct values.

  • Isolate Tests and code in module-test.cpp that is causing the issues but allow the rest
    to run. Isolated Tests and code has been commented out
    These include:

    • testing for the fmt::detail namespace. The MSVC github workflow could
      not differentiate the between the fmt::detail and fmt::v12::detail
    • tests for print and vprint, particularly those requiring stdout
      and stderr. an exception was being thrown
    • Several tests for items that have since been removed from the
      library like fmt::is_char and fmt::localtime as far as I can
      tell
  • Some of the tests appear to be duplicates of the other regular library
    tests.

    Should close Enable module test #4698

- Refactor test/CMakeLists.txt to enable testing for modules
- Refactored tests related to `fmt::format_args` that now requires lvalue
  references as opposed to direct values.
- Isolate Tests and code in module-test.cpp that is causing the issues but allow the rest
  to run. Isolated Tests and code has been commented out
  These include:
    - testing for the fmt::detail namespace. The MSVC github workflow could
      not differentiate the between the fmt::detail and fmt::v12::detail
    - tests for print and vprint, particularly those requiring stdout
      and stderr. an exception was being thrown
    - Several tests for items that have since been removed from the
      library like `fmt::is_char` and `fmt::localtime` as far as I can
      tell
- Some of the tests appear to be duplicates of the other regular library
  tests.
- `fmt.cc` removed private module due to error in clang
@MathewBensonCode MathewBensonCode marked this pull request as ready for review March 5, 2026 20:32
@MathewBensonCode MathewBensonCode marked this pull request as draft March 6, 2026 04:32
@MathewBensonCode
Copy link
Contributor Author

@vitaut, what is the scope of testing that we require for testing the module target?

Do we just want to test the public API or the full API?

The tests for the regular library involve tests for the entire library as they are able to reach into the entire code base.

With the module based library, it brings up the issue of visible and invisible APIs.

In this pull request, Ideally I have just remove the "inner" code(and some outdated code), which is being tested by the regular library anyway, and just test that the expected public functionality works.

I have commented out the bits that would be deleted if we we're to take this approach.

The other approach would be to work out how to work with modules and macro based testing frameworks.

I'm doing some experiments with this, but it may need another pull request as the changes are likely to be drastic, like moving tests into headers, or making the tests part of the module, etc.

@vitaut vitaut mentioned this pull request Mar 7, 2026
@vitaut
Copy link
Contributor

vitaut commented Mar 7, 2026

We definitely don't need to test everything. A simple sanity test that exercises a small subset of the API should be sufficient. I would recommend splitting the changes into smaller PRs, e.g. focusing on one compiler (e.g. clang) first.

@vitaut
Copy link
Contributor

vitaut commented Mar 7, 2026

That said, we shouldn't delete code from the module test if it is broken with modules. On the contrary - we should keep it as a regression test for the fix. It is OK to conditionally compile it until the fix is available.

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.

Enable module test

3 participants