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

Add tests leveraging the existing documentation #26

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

D4N
Copy link
Contributor

@D4N D4N commented Jun 6, 2020

This is a pretty huge PR consisting of a lot of individual changes, so please review them commit by commit, as otherwise the diff will be too much.

tl;dr; I have noticed that jtc has a pretty impressive documentation, but no tests. So I have written a simple python script that will run all code samples from the documentation and check if the outputs match.

The tests can be now run via python3 run_tests.py -v User\ Guide.md Walk-path\ tutorial.md README.md assuming that jtc is in PATH or in the current working directory. The test runner also supports running all commands through valgrind, simply by adding the --valgrind flag.

This has required some changes to the documentation: I had to adjust a few code samples to be a little less readable (but imho that is bearable) to keep the python script simple as it does not emulate a full shell session. Also, a bunch of outputs were no longer correct and had to be adjusted and quite a few tests failed when run with address sanitizer (these are skipped by changing the code block start from bash to bash SKIP, failures are marked with FIXME to distinguish them from tests that should never be run).

What do you think about this? Worthwhile to pursue further? If yes, then I'd like to address the following points before merging this:

TODO:

  • document how to write tests and the limitations
  • setup a CI via github actions

@ldn-softdev
Copy link
Owner

ldn-softdev commented Jun 6, 2020

Hi D4N,

tl;dr; I have noticed that jtc has a pretty impressive documentation, but no tests.

  • yes, jtc has extensive test suit using google-test framework, but I don't publish it, 'cause its code currently is somewhat sloppy and not written up to my standards - need to give it a certain time, which I currently don't have

So I have written a simple python script that will run all code samples from the documentation and check if the outputs match.

  • that's the great idea, having some publicly available auto-test suit is never a bad idea and highly welcomed and appreciated

This has required some changes to the documentation: I had to adjust a few code samples to be a little less readable (but imho that is bearable) to keep the python script simple as it does not emulate a full shell session

  • let me review it carefully.

Also, a bunch of outputs were no longer correct and had to be adjusted and quite a few tests failed when run with address sanitizer (these are skipped by changing the code block start from bash to bash SKIP, failures are marked with FIXME to distinguish them from tests that should never be run).

  • Could you use the latest source or binary? There was a couple accidental regressions introduced in the version 1.76, but they all fixed now in the latest builds. - all of the examples in User Guide should pass - all except this one:
<ab.json jtc -w'<$#:\t>v<NY>' -qqT'"{$PATH}"'
  • that one was a loophole in the prior template implementation logic and rather hacky behavior, besides it contradict the prior documented template-interpolation behavior - and it should be removed from the documentation (will do it asap).
  • All the tests also should pass using address sanitizer if compiled out the latest source.
  • Also, I'd defer covering Walk-path tutorial.md - the document is incomplete yet and based on the much older version of jtc. Once it's finished - then it could be covered and fixed whatever would be required to.

What do you think about this? Worthwhile to pursue further? If yes, then I'd like to address the following points before merging this:

  • yes, definitely let's do this (though it'll take some time on my end, as I'm currently have very limited available bandwidth).

@ldn-softdev
Copy link
Owner

ldn-softdev commented Jun 6, 2020

all of the examples in User Guide should pass - all except this one:
<ab.json jtc -w'<$#:\t>v' -qqT'"{$PATH}"'

  • actually, I rethought - interpolation of special tokens like '$PATH' should not be different than interpolation any other token, thus I consider it a bug - will file and fix soon.

@D4N D4N force-pushed the add_tests branch 8 times, most recently from fe63ea7 to 921e53c Compare June 8, 2020 21:41
User Guide.md Outdated Show resolved Hide resolved
@D4N
Copy link
Contributor Author

D4N commented Jun 11, 2020

Hi D4N,

tl;dr; I have noticed that jtc has a pretty impressive documentation, but no tests.

* yes, `jtc` has extensive test suit using google-test framework, but I don't publish it, 'cause its code currently is somewhat sloppy and not written up to my standards - need to give it a certain time, which I currently don't have

I understand that. But let me just say that I'd rather see a project with a sloppy test suite than with none at all. Also, if you make the tests public, they could be added to the CI and run on all pull requests and commits.

So I have written a simple python script that will run all code samples from the documentation and check if the outputs match.

* that's the great idea, having some publicly available auto-test suit is never a bad idea and highly welcomed and appreciated

This has required some changes to the documentation: I had to adjust a few code samples to be a little less readable (but imho that is bearable) to keep the python script simple as it does not emulate a full shell session

* let me review it carefully.

Also, a bunch of outputs were no longer correct and had to be adjusted and quite a few tests failed when run with address sanitizer (these are skipped by changing the code block start from bash to bash SKIP, failures are marked with FIXME to distinguish them from tests that should never be run).

* Could you use the latest source or binary? There was a couple accidental regressions introduced in the version 1.76, but they all fixed now in the latest builds. - all of the examples in `User Guide` should pass - all except this one:
<ab.json jtc -w'<$#:\t>v<NY>' -qqT'"{$PATH}"'

I am using the latest commit to run the tests and not a specific tag.

There are actually a few other examples that are failing, I have opened issues for some of them (#27 and #28) and will create issues for all remaining failures.

* that one was a loophole in the prior template implementation logic and rather hacky behavior, besides it contradict the [prior documented](https://github.com/ldn-softdev/jtc/blob/master/User%20Guide.md#-interpolation-of-iterables-into-a-string-template) template-interpolation behavior - and it should be removed from the documentation (will do it asap).

* All the tests also should pass using _address sanitizer_ if compiled out the latest source.

I have added a test run with ASAN+UBSAN, one without and one with valgrind to the CI.

* Also, I'd defer covering `Walk-path tutorial.md` - the document is incomplete yet and based on the much older version of `jtc`. Once it's finished - then it could be covered and fixed whatever would be required to.

Oh, I did not know that. I will remove it from the test run in the meantime (although all except 3 examples from that file passed).

What do you think about this? Worthwhile to pursue further? If yes, then I'd like to address the following points before merging this:

* yes, definitely let's do this (though it'll take some time on my end, as I'm currently have very limited available bandwidth).

No problem at all, please take the time you need to review this.

@ldn-softdev ldn-softdev marked this pull request as ready for review July 5, 2020 14:05
D4N added 3 commits July 5, 2020 21:32
clang puts a ./ in front of __FILE__, which gcc does not. To remove this
difference, we add the constexpr drop_leading_dot_slash() which removes this
difference between both compilers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants