refactor tests for invalid format items

[vasiliyk]

refactor tests for invalid format items closing issue #193

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.17%. Comparing base (5f68b8f) to head (08f87ca).
Report is 2 commits behind head on latest.

Additional details and impacted files

@@            Coverage Diff             @@
##           latest     #474      +/-   ##
==========================================
- Coverage   90.42%   90.17%   -0.25%     
==========================================
  Files          47       47              
  Lines        4385     4388       +3     
  Branches      587      588       +1     
==========================================
- Hits         3965     3957       -8     
- Misses        286      288       +2     
- Partials      134      143       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[vasiliyk]

a few changes that I forgot earlier:

1. include moved form /test/helper/fixture.cpp to /include/test/helper/fixture.hpp
2. include added to stumpless_private.yml

[vasiliyk]

@goatshriek, could you surmise why tests fail?

[goatshriek]

I can't identify the exact cause from a quick glance. Are you seeing failures when you run the tests yourself?

There may be entries in the directory walk that you aren't expecting, based on some of the error messages being an "unknown file". Perhaps that would be the best place to start troubleshooting.

[vasiliyk]

Are you seeing failures when you run the tests yourself?

no, unfortunately, I have no errors on my Mac. I will add line terminators to the files that I added in directory stumpless/test/corpora/invalid_param_name

[vasiliyk]

ok, found it - dirent.h include is not available for Windows, I need to add code for Win additionally.

[vasiliyk]

I made code Win and Unix/Linux/MacOS compatible and CLang pure (without C++ elements).
I am intrested to see Win/Linux tests, please allow tests execution.

[vasiliyk]

Joel, some Windows tests are failing due to attempt to load 'dirent.h'.

D:\a\stumpless\stumpless\test\helper\fixture.cpp(20,12): fatal error C1083: Cannot open include file: 'dirent.h': No such file or directory [D:\a\stumpless\stumpless\test_helper_fixture.vcxproj]

However, I have the following code:

#ifndef HAVE_WINDOWS_H
  #include <dirent.h>
#endif

Can it be a case where HAVE_WINDOWS_H is not defined for Windows test execution?

[goatshriek]

First of all, thanks very much for putting together this change!

The HAVE_WINDOWS_H symbol is defined in the private configuration header, which is not exposed in the tests. This is why it is still being included in your tests, despite it being defined during the build itself. The header check in the Static Analysis would warn you about this.

Taking a step back though, it seems that filesystem operations in C++ can be hard to make portable, which isn't something I had anticipated.

Currently, the project does not impose any C++ version restrictions, instead simply inheriting from Google Test, which currently requires C++11. Newer versions of C++ (from C++17 onwards) have <filesystem>, which seems like it will make this much easier for you given this Stack Overflow example.

Fortunately we're working on a major version release right now, so a major change like this is acceptable. Furthermore, I intend to migrate to Google FuzzTest as part of #335. FuzzTest requires C++17 anyway, so this standard will be in bounds for the C++ version required for testing. You may need to add a C++ standard statement to CMake for now to get things to build. If so you can add this anywhere, and I will refactor it when I update CMake and integrate FuzzTest later on.

tl;dr

Refactor the file walk to use C++ <filesystem> which should resolve portability concerns and make your code much simpler.

I've made a few other suggestions in my review as well.

[vasiliyk]

Joel, load_corpus_folder() logic converted to c++17 to simplify code

[vasiliyk]

Based on error messages I guess the issue was in FS namespace for Windows.
I don't have Windows machine so I pushed the change to see the test results.

[vasiliyk]

Joel,
all Windows and MacOS builds passed tests, Linux builds on c++ passed as well.

What should be the next step?

[goatshriek]

All of the tests need to pass. The Linux "with C++" tests are aimed at the C++ bindings, and are probably passing because they do not rely on the new code yet.

There is code in tools/cmake/test.cmake that sets the C++ standard - you may need to modify this to fix the builds. I recommend testing this in a container or virtual machine so that you can be sure it is fixed.

[vasiliyk]

I set c++17 standard in tools/cmake/test.cmake in addition to CMakeList.txt.

Great idea to use a container, I will setup it up once, and then it will speed-up testing for me.

[vasiliyk]

Joel,
there is a problem only with static analysis, I don't see the issue on my machine:

vasiliy@night stumpless % tools/check_headers/check_headers.rb "include/**/*.h*" "include/**/**/*.h"
warning: deprecated term stumpless_destroy_entry used in include/stumpless/entry.h
warning: deprecated term stumpless_destroy_element used in include/stumpless/element.h
warning: deprecated term stumpless_destroy_entry used in include/stumpless/entry.h
warning: deprecated term stumpless_destroy_element used in include/stumpless/element.h

Should I adjust stumpless_private.yml based on the details I see on github?

[goatshriek]

The "warning" lines can be ignored. The cause for the failure is the messages below the warnings:

test/function/element.cpp: unused include test/helper/fixture.hpp
test/function/entry.cpp: 'string' requires inclusion of string
test/function/param.cpp: 'string' requires inclusion of string
test/function/param.cpp: unused include test/helper/fixture.hpp
test/helper/fixture.cpp: unused include filesystem

You should only need to add an entry to tools/check_headers/cpp_standard_library.yml for the filesystem include, but other than that you should be able to resolve the issues by taking the suggested action in each message.

[vasiliyk]

headers configuration updated.

[goatshriek]

Thank you once again for helping refactor the test suite, which is often overlooked. Your persistence in sticking with this through all of the requested modifications and portability concerns is especially commendable!