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 many valid and invalid test JSON files #919

Merged
merged 2 commits into from
Jul 28, 2024

Conversation

xexyl
Copy link
Contributor

@xexyl xexyl commented Jul 28, 2024

With MUCH GRATITUDE (THANK YOU!) to the JSONTestSuite repo (https://github.com/nst/JSONTestSuite) maintainers we have added many good and bad JSON files of more unique kinds (as in kinds that some parsers get wrong, both false positive and false negative) to the jparse/test_jparse/test_JSON subdirectories (good/ and bad/) and all is good.

A note is that this does slow down make test, make prep (and make slow_prep) a bit but this is worth it because we can now be even more sure that everything is in order if by chance an error is introduced into the parser. We did not (by an oversight) document that new GitHub workflows have been added to run tests on commit and push that will show any problems before a commit is merged to the master branch (well if Landon does a push that is another matter entirely) so that is another recent safety mechanism.

The jparse/README.md file has been updated: moved the history section to the very bottom and added the section on the testing procedure (I also slightly amended the heading of the additional tools that are VERY INCOMPLETE and will very likely be rewritten almost entirely) with a warning so it stands out even more.

With MUCH GRATITUDE (THANK YOU!) to the JSONTestSuite repo
(https://github.com/nst/JSONTestSuite) maintainers we have added many
good and bad JSON files of more unique kinds (as in kinds that some
parsers get wrong, both false positive and false negative) to the
jparse/test_jparse/test_JSON subdirectories (good/ and bad/) and all is
good.

A note is that this does slow down make test, make prep (and make
slow_prep) a bit but this is worth it because we can now be even more
sure that everything is in order if by chance an error is introduced
into the parser. We did not (by an oversight) document that new GitHub
workflows have been added to run tests on commit and push that will show
any problems before a commit is merged to the master branch (well if
Landon does a push that is another matter entirely) so that is another
recent safety mechanism.

The jparse/README.md file has been updated: moved the history section to
the very bottom and added the section on the testing procedure (I also
slightly amended the heading of the additional tools that are VERY
INCOMPLETE and will very likely be rewritten almost entirely) with a
warning so it stands out even more.
@xexyl
Copy link
Contributor Author

xexyl commented Jul 28, 2024

I forgot to add that I added a call to date in make test: for starting and ending. Seems helpful. Hopefully I did not make a syntax error .. meant to check but then forgot (added it at the last). If there is I'll fix it. Then going afk a while.

Add that the make test, make prep and make slow_prep rules now have (as
of the last commit) a timestamp at beginning and end of the rules (this
is very helpful at times).

I also added that we recently (thanks @SirWumpus!) added a workflow so
that we can detect a problem before (unless Landon does the commit of
course :-) ) a commit is merged into master.
Copy link
Contributor

@lcn2 lcn2 left a comment

Choose a reason for hiding this comment

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

They provided lots of good test cases. Thanks for adding them, @xexyl

It is vey nice to see that the JOSN parser passed all of their tests!

@lcn2 lcn2 merged commit 0eef3e7 into ioccc-src:master Jul 28, 2024
3 checks passed
@xexyl
Copy link
Contributor Author

xexyl commented Jul 28, 2024

They provided lots of good test cases. Thanks for adding them, @xexyl

They did. It seemed like it would be better to add the files rather than try and keep it separate. This way if anything changes by error we can become aware of it quickly.

It is vey nice to see that the JOSN parser passed all of their tests!

Definitely! I feel the same way. I suspected we would be in good shape but now we're certain of it!

@xexyl xexyl deleted the json-test-files branch July 28, 2024 17:35
@lcn2
Copy link
Contributor

lcn2 commented Jul 28, 2024

When it comes time to make jparse a public repo (we recommend doing this after the Great Fork Merge just in case we need to make additional minor tweaks to the this repo), we recommend issuing a pull request to the test site showing how well jparse did. 👍

@xexyl
Copy link
Contributor Author

xexyl commented Jul 29, 2024

When it comes time to make jparse a public repo (we recommend doing this after the Great Fork Merge just in case we need to make additional minor tweaks to the this repo)

Oh I have no intention of doing this until after the merge: not only so that we can focus on that other repo but also it might even be better to wait until after the next IOCCC. Even more it might be better to wait until we rewrite the three tools and get them in good shape.

It is true I would LIKE to do it just to 'have it' but I think it would be unwise at this time. Of course it might be that I do do it before those tools are made but whether that comes after the next IOCCC or not (the adding to the separate repo) I do not know - at least not yet.

... we recommend issuing a pull request to the test site showing how well jparse did. 👍

It'd be nice but that requires figuring out their system. It might be easy enough but whether it's worth the effort or not I do not know. Still it's a a nice idea that might be something to do later on yes.

--

I have other things to do and then will be on a call. After that it's POSSIBLE I'll have a bit of time but it won't be very much time anyway so it's likely that getting 1998 done today was all I'll do. We'll see!

@lcn2
Copy link
Contributor

lcn2 commented Jul 29, 2024

... we recommend issuing a pull request to the test site showing how well jparse did. 👍

It'd be nice but that requires figuring out their system. It might be easy enough but whether it's worth the effort or not I do not know. Still it's a a nice idea that might be something to do later on yes.

Agreed. Nevertheless it will be a good advertisement for jparse.

@xexyl
Copy link
Contributor Author

xexyl commented Jul 29, 2024

... we recommend issuing a pull request to the test site showing how well jparse did. 👍

It'd be nice but that requires figuring out their system. It might be easy enough but whether it's worth the effort or not I do not know. Still it's a a nice idea that might be something to do later on yes.

Agreed. Nevertheless it will be a good advertisement for jparse.

That is a very good point actually yes.

Well we can worry about that later on. Still a lot to do before then!

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