Regression test for #3070 is not being run and fails when enabled #3377

falbrechtskirchinger · 2022-03-05T12:44:47Z

Working on some unit test improvements, I discovered at least two unit tests that are not being run. This issue is about

#if JSON_HAS_FILESYSTEM || JSON_HAS_EXPERIMENTAL_FILESYSTEM
    SECTION("issue #3070 - Version 3.10.3 breaks backward-compatibility with 3.10.2 ")
    {
        static_assert(false); // Added to confirm this test is never run
        nlohmann::detail::std_fs::path text_path("/tmp/text.txt");
        json j(text_path);

        const auto j_path = j.get<nlohmann::detail::std_fs::path>();
        CHECK(j_path == text_path);

        CHECK_THROWS_WITH_AS(nlohmann::detail::std_fs::path(json(1)), "[json.exception.type_error.302] type must be string, but is number", json::type_error);
    }
#endif

added in 6d31159.

nlohmann::detail::std_fs::path(json(1)) fails to compile, because the constructor call is ambiguous.

@theodelrieu @nlohmann Was/is this expected to work?

(The fact, that this test isn't being run will be fixed by an upcoming PR.)

The text was updated successfully, but these errors were encountered:

Move definition of common macros (e.g., JSON_HAS_CPP_*) into macros.hpp header file and update unit test sources. Incidentally enables the regression tests for nlohmann#2546 and nlohmann#3070. A CHECK_THROWS_WITH_AS in nlohmann#3070 was disabled which is tracked here: nlohmann#3377

Move definition of common macros (e.g., JSON_HAS_CPP_*) into macros.hpp header file and update unit test sources. Incidentally enables the regression tests for nlohmann#2546 and nlohmann#3070. A CHECK_THROWS_WITH_AS in nlohmann#3070 was disabled, which is tracked here: nlohmann#3377

Move definition of common macros (e.g., JSON_HAS_CPP_*) into macros.hpp header file and update unit test sources. Incidentally enables the regression tests for nlohmann#2546 and nlohmann#3070. A CHECK_THROWS_WITH_AS in nlohmann#3070 was disabled which is tracked in nlohmann#3377 and a line in from_json(..., std_fs::path&) was marked with LCOV_EXCL_LINE.

falbrechtskirchinger · 2022-03-08T09:24:03Z

Clarification: If I remember correctly the ambiguity arises from std::fs::path accepting both std::string and std::fs::path (copy ctor, obviously) and basic_json<> can implicitly convert to both. I don't see a way to resolve this ambiguity.

@nlohmann Thoughts?

Again, this has apparently never worked as the test was never executed.

nlohmann · 2022-03-08T10:19:51Z

Thanks for clarifying! I'll think about this. Good to know that it never worked - this allows us to make changes without breaking existing implementations ;-)

nlohmann · 2022-03-08T12:46:39Z

I am confused - locally, the test is executed and succeeds... (Apple clang version 13.0.0)

nlohmann · 2022-03-08T13:08:58Z

So it seems the test is indeed not executed on Windows - maybe the filesystem detection is broken and all compilers are skipped. The test is executed on macOS and Linux for some compilers. However, not for all C++17 enabled compilers. 🤔

falbrechtskirchinger · 2022-03-08T13:18:12Z

While I didn't dig into why it is now running in my builds (#3380). I consolidated the test macros into a separate header and copied them over from include/nlohmann/detail/macro_scope.hpp. The unit test embedded macros were either defined incorrectly or missing.

I'm also working on printing out build information with each test to better understand which test are compiled using which flags and standard version, etc.

nlohmann · 2022-03-08T13:20:09Z

Good point. I think outputting some of the defines makes sense. File cmake/download_test_data.cmake is currently used for this. Not optimal, but better than nothing.

falbrechtskirchinger · 2022-03-08T13:27:35Z

Well, I'm definitely going overboard but you can read all about the how and why in my PR soon. Hopefully, that is. After 20+ red CI runs they have to turn green eventually, right? 😉

Move definition of common macros (e.g., JSON_HAS_CPP_*) into macros.hpp header file and update unit test sources. Incidentally enables the regression tests for nlohmann#2546 and nlohmann#3070. A CHECK_THROWS_WITH_AS in nlohmann#3070 was disabled which is tracked in nlohmann#3377 and a line in from_json(..., std_fs::path&) was marked with LCOV_EXCL_LINE.

Incidentally enables the regression tests for nlohmann#2546 and nlohmann#3070. A CHECK_THROWS_WITH_AS in nlohmann#3070 was disabled which is tracked in nlohmann#3377 and a line in from_json(..., std_fs::path&) was marked with LCOV_EXCL_LINE.

* Refactor unit test creation Add functions for creating tests and to supply test- and standard-specific build settings. Raises minimum CMake version to 3.13 in test directory. json_test_add_test_for( <file> MAIN <main> [CXX_STANDARDS <version_number>...] [FORCE]) Given a <file> unit-foo.cpp, produces test-foo_cpp<version_number> if C++ standard <version_number> is supported by the compiler and thesource file contains JSON_HAS_CPP_<version_number>. Use FORCE to create the test regardless of the file containing JSON_HAS_CPP_<version_number>. Test targets are linked against <main>. CXX_STANDARDS defaults to "11". json_test_set_test_options( all|<tests> [CXX_STANDARDS all|<args>...] [COMPILE_DEFINITIONS <args>...] [COMPILE_FEATURES <args>...] [COMPILE_OPTIONS <args>...] [LINK_LIBRARIES <args>...] [LINK_OPTIONS <args>...]) Supply test- and standard-specific build settings. Specify multiple tests using a list e.g., "test-foo;test-bar". Must be called BEFORE the test is created. * Use CMAKE_MODULE_PATH * Don't undef some macros if JSON_TEST_KEEP_MACROS is defined * Use JSON_TEST_KEEP_MACROS Incidentally enables the regression tests for #2546 and #3070. A CHECK_THROWS_WITH_AS in #3070 was disabled which is tracked in #3377 and a line in from_json(..., std_fs::path&) was marked with LCOV_EXCL_LINE. * Add three-way comparison feature test macro * Disable broken comparison if JSON_HAS_THREE_WAY_COMPARISON * Fix redefinition of inline constexpr statics Redelcaration of inline constexpr static data members in namespace scope was deprecated in C++17. Fixes -Werror=deprecated compilation failures. * Fix more test build failures due to missing noexcept * CI: update cmake_flags test to use CMake 3.13 in test directory Also change default for JSON_BuildTests option to depend on CMake version. * CI: turn *_CXXFLAGS into CMake lists * CI: use JSON_TestStandards to set CXX_STANDARD * CI: pass extra CXXFLAGS to standards tests

Closes nlohmann#3377 and nlohmann#3382.

Restore the previously disabled check for regression nlohmann#3070 on all compilers but MSVC. To summarize the issue: Given namespace fs = std::filesystem. On MSVC attempting to construct an fs::path from json results in an ambiguous overload resolution because fs::path can be constructed from both a std::string as well as another fs::path. To the best of my knowledge there is no way to fix an ambiguous overload situation involving a type we do not control and with json implicitly converting to both std::string and fs::path. Re-enabling the check where it compiles and keeping it disabled for MSVC is the best we can do. Closes nlohmann#3377 and nlohmann#3382.

Restore the previously disabled check for regression #3070 on all compilers but MSVC. To summarize the issue: Given namespace fs = std::filesystem. On MSVC attempting to construct an fs::path from json results in an ambiguous overload resolution because fs::path can be constructed from both a std::string as well as another fs::path. To the best of my knowledge there is no way to fix an ambiguous overload situation involving a type we do not control and with json implicitly converting to both std::string and fs::path. Re-enabling the check where it compiles and keeping it disabled for MSVC is the best we can do. Closes #3377 and #3382.

falbrechtskirchinger added the kind: bug label Mar 5, 2022

falbrechtskirchinger mentioned this issue Mar 6, 2022

Improve unit testing (Part 1) #3380

Merged

falbrechtskirchinger mentioned this issue Mar 7, 2022

Add build step for ICPC #3229

Closed

nlohmann added a commit that referenced this issue Mar 8, 2022

⚗️ reproducing #3377

749b3b7

nlohmann mentioned this issue Mar 8, 2022

Reproducing #3377 #3382

Closed

nlohmann added the state: help needed the issue needs help to proceed label Mar 8, 2022

falbrechtskirchinger added a commit to falbrechtskirchinger/json that referenced this issue Apr 5, 2022

Restore disabled check for nlohmann#3070 (except on MSVC)

30b4e13

Closes nlohmann#3377 and nlohmann#3382.

falbrechtskirchinger mentioned this issue Apr 5, 2022

Restore disabled check for #3070 (except on MSVC) #3421

Merged

nlohmann added solution: proposed fix a fix for the issue has been proposed and waits for confirmation release item: 🔨 further change and removed state: help needed the issue needs help to proceed labels Apr 6, 2022

nlohmann self-assigned this Apr 6, 2022

nlohmann added this to the Release 3.10.6 milestone Apr 6, 2022

nlohmann closed this as completed in #3421 Apr 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Regression test for #3070 is not being run and fails when enabled #3377

Regression test for #3070 is not being run and fails when enabled #3377

falbrechtskirchinger commented Mar 5, 2022 •

edited by nlohmann

Loading

falbrechtskirchinger commented Mar 8, 2022 •

edited

Loading

nlohmann commented Mar 8, 2022

nlohmann commented Mar 8, 2022

nlohmann commented Mar 8, 2022 •

edited

Loading

falbrechtskirchinger commented Mar 8, 2022

nlohmann commented Mar 8, 2022

falbrechtskirchinger commented Mar 8, 2022

Regression test for #3070 is not being run and fails when enabled #3377

Regression test for #3070 is not being run and fails when enabled #3377

Comments

falbrechtskirchinger commented Mar 5, 2022 • edited by nlohmann Loading

falbrechtskirchinger commented Mar 8, 2022 • edited Loading

nlohmann commented Mar 8, 2022

nlohmann commented Mar 8, 2022

nlohmann commented Mar 8, 2022 • edited Loading

falbrechtskirchinger commented Mar 8, 2022

nlohmann commented Mar 8, 2022

falbrechtskirchinger commented Mar 8, 2022

falbrechtskirchinger commented Mar 5, 2022 •

edited by nlohmann

Loading

falbrechtskirchinger commented Mar 8, 2022 •

edited

Loading

nlohmann commented Mar 8, 2022 •

edited

Loading