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.

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

Unit testing framework #266

Open
LecrisUT opened this issue May 9, 2023 · 10 comments · May be fixed by #306
Open

Unit testing framework #266

LecrisUT opened this issue May 9, 2023 · 10 comments · May be fixed by #306

Comments

@LecrisUT
Copy link
Collaborator

LecrisUT commented May 9, 2023

How about using catch2 or any other unit testing frameworks. Nlohmann seems to be using doctest for that, probably because of the limitations it had when catch2 was header-only, but these days I don't think they are relevant anymore and catch2 has made quite nice cmake integrations for it.

@pboettch
Copy link
Owner

pboettch commented May 9, 2023

Instead of my custom bash-script plus ctest?

The problem is the json-schema-validator-test-suite. This is imported here and I really like the idea of reading from files, the schema and the instance.

@LecrisUT
Copy link
Collaborator Author

Hmm, so all unit testing is through test-suite? I was thinking of the individual folders over there. We could have both, where catch can handle anythings that is not covered by json-schema-validator-test-suite.

I think the test-suite should be converted to a submodule so it is easier to keep up to date. I can design something custom using either the proper cli interface or using catch2. The latter will be easier to create fine-grained control in the future e.g. experimental drafts.

@pboettch
Copy link
Owner

There is no unit-testing. Only functional testing. By using the test-suite and, in a similar manner (files) some test-cases which were mostly issues seen during the life of the project.

What is really missing is a coverage analysis, which points us to the weaknesses not covered by issues and functional tests.

@pboettch
Copy link
Owner

I strongly believe we can achieve 100% coverage by using the test-suite, issue-test and some corner case test written by hand. At this state of the library.

@LecrisUT
Copy link
Collaborator Author

Well, I've enabled the coverage, and indeed it was showing 90℅ coverage. Will have to wait for it to be merged in main for the full report though. After that let's double check if we need any additional testing framework or if we cover it all via test-suite.

But about test-suite, what about making it a submodule?

@pboettch
Copy link
Owner

Why not use fetchcontent to get the test-suite, if required (ie. enabled via cmake)?

@LecrisUT
Copy link
Collaborator Author

Oh, interesting point. I thought fetchcontent is only usable for cmake projects, but let me do a bit of researching and I'll get back to you on this.

@LecrisUT
Copy link
Collaborator Author

Thanks for the idea. FetchContent does indeed work for that. There is one design problem though, how to make this compatible with Fedora packaging. For that, it must be buildable without internet connection.

My thought is to still use git submodules and make a release GH action that includes those submodules. Then on the cmake side, I can make a simple check if there is internet connection and if not use the submodule location as a fallback. Or, keep the logic simple (always defaulting to downloaded FetchContent and fail if no internet connection) and keep it as an advanced configuration method (manually setting FETCHCONTENT_SOURCE_DIR_TESTSUITE).

I can make a simple script to add to pre-commit so that the submodule version and the FetchContent one are always in sync.

@pboettch
Copy link
Owner

Or we leave it as it is and just copy the test-suites (for the supported version) here from time to time ;-)

@LecrisUT
Copy link
Collaborator Author

True, still needs to be redesigned so that the contents are untouched and it is easier to update it. (would also make it possible to add the tests as experimental in the CI ;) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants