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.

Sign up for GitHub

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

Jump to bottom

Fix unit tests that were silently skipped or crashed (depending on the compiler) #1176

Merged
merged 2 commits into from
Aug 14, 2018
Merged

Fix unit tests that were silently skipped or crashed (depending on the compiler) #1176

merged 2 commits into from
Aug 14, 2018

Conversation

grembo
Copy link

@grembo grembo commented Jul 29, 2018

Make section names unique in loops, as catch doesn't support duplicate sections, see also catchorg/Catch2#816 (comment)

As a result, when built with gcc, loop iterations were skipped. When
built with clang, the test aborted with an assertion in catch.hpp
line 6222.

This also addresses the issues discussed here:
#1032 (comment)

and here:
catchorg/Catch2#1241

As some of the unit tests which never ran before failed now, I added an exclude list for one of the tests (this is due to nlohmann-json ordering dictionaries by key, while the output files it compares against kept the key order of the json files).

Also, the way the tests were written, they would now take a long time, therefore I moved parsing the source into individual sections (more code, but 99% less parsing operations for the same tests).

Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

Please don't

  • The C++11 support varies between different compilers and versions. Please note the list of supported compilers. Some compilers like GCC 4.8 (and earlier), Clang 3.3 (and earlier), or Microsoft Visual Studio 13.0 and earlier are known not to work due to missing or incomplete C++11 support. Please refrain from proposing changes that work around these compiler's limitations with #ifdefs or other means.
  • Specifically, I am aware of compilation problems with Microsoft Visual Studio (there even is an issue label for these kind of bugs). I understand that even in 2016, complete C++11 support isn't there yet. But please also understand that I do not want to drop features or uglify the code just to make Microsoft's sub-standard compiler happy. The past has shown that there are ways to express the functionality such that the code compiles with the most recent MSVC - unfortunately, this is not the main objective of the project.
  • Please refrain from proposing changes that would break JSON conformance. If you propose a conformant extension of JSON to be supported by the library, please motivate this extension.
  • Please do not open pull requests that address multiple issues.

Michael Gmelin added 2 commits July 29, 2018 10:44
Make section names unique in loops, as catch doesn't support duplicate
d5aaeb4 
sections, see also catchorg/Catch2#816 (comment)

As a result, when built with gcc, loop iterations were skipped. When
built with clang, the test aborted with an assertion in catch.hpp
line 6222.

This also addresses the issues discussed here:
nlohmann#1032 (comment)

and here:
catchorg/Catch2#1241

Please note that this introduces new problems, as some of
the unit tests fail now - the library stores keys in
lexographical order, while the cbor/msgpack/ubjson examples
store them in original order.
Exclude bytewise comparison in certain tests.
05b27e8 
These tests never worked - they weren't run before
d5aaeb4.

Note that these tests would fail because of this library
ordering dictionary keys (which is legal). So changing the
input files (or modifying stored cbor/msgpack/ubjson files)
would make the tests work and they could get removed from
"exclude_packaged".

Also move parsing of files in these unit tests to within
the inner sections, so that they're only parsed
number_of_files * number_of_sections instead of
number_of_files * number_of_files * number_of_sections
(so, instead of close to 100k parses about 700).
@grembo grembo changed the title Fix unit tests that weren't silently skipped or crashed (depending on the compiler) Fix unit tests that were silently skipped or crashed (depending on the compiler) Jul 29, 2018
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 05b27e8 on grembo:develop into 3760a38 on nlohmann:develop.

@nlohmann
Copy link
Owner

Thanks a lot for the PR. I shall review it shortly.

@nlohmann nlohmann self-requested a review July 30, 2018 12:54
nlohmann
nlohmann approved these changes Aug 14, 2018
View reviewed changes
Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann added this to the Release 3.1.3 milestone Aug 14, 2018
@nlohmann nlohmann self-assigned this Aug 14, 2018
@nlohmann nlohmann merged commit 861ee40 into nlohmann:develop Aug 14, 2018
@nlohmann
Copy link
Owner

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlohmann nlohmann nlohmann approved these changes

Assignees

@nlohmann nlohmann

Labels
None yet
Projects
None yet
Milestone
Release 3.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@grembo @coveralls @nlohmann