mayah / tinytoml

A header only C++11 library for parsing TOML
BSD 2-Clause "Simplified" License
167 stars 31 forks source link

Expand testsuite with builder/writer tests that enumerate success documents. #15

Closed veeg closed 7 years ago

veeg commented 7 years ago

This branch contains two new test executables that test builder/writer interfaces (which, are somewhat lacking..)

Commit 60efde0 fixes proper escaping of keys when outputting the value to ostream.

The builder test (BuilderTest.build_parse_datetime_01) is currently failing. I'm simply assuming this is due to a parser bug (which I have yet to look into. Priorities..)

I'm in the process of implementing better write support (more formatting options, as well as per Value). I'm positive this will resolve #11 as well.

Please review and consider merging.

veeg commented 7 years ago

That's odd. The writer_test is failing as well on Travis, but I doesn't on my end. I've tried both g++ and clang.

Can you perhaps change the travis script to include output?

CTEST_OUTPUT_ON_FAILURE=1 make check
mayah commented 7 years ago

Added CTEST_OUTPUT_ON_FAILURE=1 in https://github.com/mayah/tinytoml/commit/0f655021c69d925b2349c856de142dd65262d5b7

veeg commented 7 years ago

Current failure is this:

/home/travis/build/mayah/tinytoml/src/builder_test.cc:22: Failure
Value of: result
  Actual: false
Expected: true
VALUE CONTENTS:
date1 = 1979-05-27T07:32:00Z
date2 = 1979-05-27T00:32:00Z
date3 = 1979-05-27T00:32:00Z
date4 = 1979-05-27T00:00:00Z
FILE CONTENTS:
date1 = 1979-05-27T07:32:00Z
date2 = 1979-05-27T07:32:00Z
date3 = 1979-05-27T07:32:00Z
date4 = 1979-05-27T00:00:00Z
[  FAILED  ] BuildTest.build_parse_datetime_01 (0 ms)

Do you have any idea what is wrong?

Please review and consider fix for failing test. I'd like to squash before merge.

veeg commented 7 years ago

Style fixes for both builder_test and toml.h corrected (in 41dfc8c and 771499f respectively).

Added weak definitions to all build.h functions in 538f9b9. Thanks for the heads up, gave me an opportunity to read up on inline again.

If you see more thoroughly on the parsed output from the success file datetime-01.toml in the failing test, you can see that for date1, date2 and date3 has somehow parsed all hours to be 07. Therefore, I think there is a more hidden bug in that parser code that results in this test failing. Of course, adding the microseconds to date3 is correct with regards to the actual success file.

I will leave this up to you how and what to proceed with. I don't have that much time to prioritize hunting down this inconsistency with regards to tinytoml, since there are more pressing writer features I need to implement (which I need for another, pressing project). My motivation to write these tests were to have a baseline for a valid parse/output before writing more formatting features (and tests ).

One option is of course to just disable the failing builder_test all together, just so the tests all pass. But I believe that speaks a bit too loud to the broken window theory. You have write access to my pull-request branch.

mayah commented 7 years ago

Anyway, thanks for the pull request. I'm going to merge your PR manually (with disabling failing tests and fixing style a bit) today.

mayah commented 7 years ago

Merged manually.

https://github.com/mayah/tinytoml/commit/7330911f1760c55feed5c60e59c73bad80ef3882

mayah commented 7 years ago

Looking at the source code more, I found you didn't consider timezone (-07:00 in datetime-01.toml) when building toml::Time. So builder_test was failing.

I've fixed it, but when writing toml::Value back, timezone information is currently lost... In toml version 0.2.0, timezone must be always GMT, so it wasn't a problem :(

veeg commented 7 years ago

Oh my. I completely misunderstood the datetime timezone semantic. Ignore my previous comments regarding datetime.

Thanks for the merge and work you've done on this library.

RobertBColton commented 7 years ago

After this PR I have the following error compiling with MinGW on Windows 10:

toml.h: In member function 'void toml::Value::write(std::ostream*, const string&, int) const':
toml.h:1298:25: error: 'gmtime_r' was not declared in this scope
         gmtime_r(&tt, &t);
                         ^
gcc version 4.8.2 (GCC)
mayah commented 7 years ago

@RobertBColton Could you file a new issue?

I heard Visual Studio does not have gmtime_r but mingw does. https://github.com/mayah/tinytoml/blob/master/include/toml/toml.h#L228 Is that changed somewhere? (Or our code is wrong?)

RobertBColton commented 7 years ago

Please see #25 I've tried to find as much info as I can. I'll keep looking but it mainly looks to be a MinGW issue at this point, MSVC works just fine. I should mention I am not really building with MinGW, I just used it because it was quick to open from my desktop and I wanted to see if I could close the formatting issue for you.

RobertBColton commented 7 years ago

Also please check #25 again if you already looked at it today, I found a simple work around that fixes it that I'll think you'll want.