Add libyaml-parser-error.t to test invalid YAML

[perlpunk]

Add script to update test lists

[ingydotnet]

Hi @perlpunk,

Could you comment about what problem you are solving here?

ie What's the current state of things, what are the problems, and what features does your solution provide.

I took a look at the code, but waas not able to get the problem/solution clear in my head.

Thanks!

[perlpunk]

Currently, make test-suite tests valid cases from yaml-test-suite.
I added libyaml-parser-error.t to test that invalid YAML is correctly parsed as invalid.
This helps to ensure that future development does not introduce bugs.

[perlpunk]

the update-test-list.sh is just a utility to put all current tests into the libyaml-{parser,emitter,parser-error}.list to check if there are new tests passing that didn't pass before.

[ingydotnet]

Hmm. The problem here is that keeping the list in the master branch of yaml/libyaml means that we are going to have new commits for every change to the test-suite. I would like to see only solid change commits go into master.

I would propose that we keep the list on a separate branch. The relatively new git worktree command lets you checkout a branch as a subdir. The test suite could then checkout that branch before running tests.

If you are concerned about the master and test-suite-data branches being in sync, Id add a file to the test-suite-data branch that contains 2 sha1s:

• the commit of the yaml-test-suite synced from
• the HEAD of yaml/libyaml master at time of commit

Then the test suite can easily report when things are not up to date.

If we do this right, then we only add one commit to master to set this up.

[perlpunk]

I don't understand the problem. The update script has to be called manually.
There are already existing lists libyaml-{parser,emitter}.list. I'm not sure why adding a list for invalid tests now should be a problem.

[ingydotnet]

@perlpunk This list was added by me, but I am realizing that it was not a good idea to add this to master, at least during a time when I expect a massive influx to the test suite.

make test-suite is really just a dev only thing, and to flood (I'm imagining this would happen) master with commits that are only test related, feels wrong to me now.

So far we have only update the list twice. I think it would be a good idea to move the list to a branch.

This was my fault for setting it up badly, and I can make the correction. @perlpunk if you want to do it, I could help you spec it out more, if needed.

Cheers, and sorry for the confusion.

[ingydotnet]

@perlpunk while I'm thinking of this, probably the best thing to do here is move all the logic that deals with testing the test-suite to a branch called test-suite and then in master all we have is a Makefile rule that looks like this:

test-suite:
        git worktree add test/suite test-suite
        make -C test/suite test

The branch could assume that it would be checked out under the test/suite subdir, when referencing the libyaml code in the main directory.

[perlpunk]

I won't have time for this in the next weeks.

[ingydotnet]

@perlpunk ok, I'll give it a shot. I can use this PR and just change it over to keep the content on a new branch.

Thanks!

[ingydotnet]

After taking a look at the code, it's almost set up perfectly already. All that needs to happen is to move the tests/run-test-suite to a branch called run-tests-suite and then add a dependency to the test-suite Makefile rule for tests/run-test-suite.

@perlpunk I'll make these changes on a branch and then let you decide if you want to add them to this PR (instead of making a new PR that includes this one).

[ingydotnet]

@perlpunk https://github.com/yaml/libyaml/tree/issue-81 adds one commit to master, removing the tests/run-test-suite/ dir (now on branch run-test-suite). The new branch has your fixes from this PR.

Let me know if this works for you. After this you can update the run-test-suite branch without needing to go through this PR process, since it is now decoupled from master development.

[perlpunk]

On a fresh clone I get this:

% git checkout issue-81
% ./bootstrap
% make
% make test
% make test-suite
git worktree add tests/run-test-suite run-test-suite
fatal: invalid reference: run-test-suite
Makefile:899: recipe for target 'tests/run-test-suite' failed
make: *** [tests/run-test-suite] Error 128

[ingydotnet]

@perlpunk, fixed in b556cfb.

[perlpunk]

and to flood (I'm imagining this would happen) master with commits that are only test related, feels wrong to me now

just wanted to note that it was not my plan to "flood" libyaml with commits.

[ingydotnet]

@perlpunk I wasn't being accusatory. It's inevitable that for every set of changes we make to the test suite, we'd want to update libyaml. Now we can do that without having a steady (whatever rate that implies) of commits like 61dd999#diff-ff771719d73db4188f03896a66c1f045

[perlpunk]

make test-suite runs now, confirmed by travis-ci.

Some notes...

• This adds another level of indirection: worktree add run-test-suite -> clone yaml-test-suite
• In the future we want to pin yaml-test-suite releases, so that older libyaml commits can be tested reliably
• Now we also have to pin run-test-suite commits to the master branch for the same reason
• Having the test list in a branch is one thing, but why also have the test infrastructure (.t scripts, Makefile etc.) there?

[perlpunk]

Alternative:

• Keep test lists in master
• Pin yaml-test-suite release in master like we planned
• Regularly update yaml-test-suite and the test lists in a normal branch
• From time to time, when necessary, squash and merge this branch into master

[perlpunk]

just wanted to note that it was not my plan to "flood" libyaml with commits.

I wasn't being accusatory.

I simply wanted to point out that it wasn't my plan to commit every yaml-test-suite change in libyaml (master).
(I do this regularly for YAML::PP which is in development, because often the new test cases are about the feature I develop.)

[perlpunk]

As an example, take the invalid escaping of a single quote \' in double quoted strings.
There had not been a specific test for that. I added a test for it to the test-suite.
Ideally, we would have a skip list, and when fixing such a bug, we remove the test id from the skip list. This serves also as a documentation which exact problem was solved.

So I think, we should also go back to a black list instead of a white list in the future. When pinning yaml-test-suite releases, this shouldn't be a problem anymore.
Thoughts?

Edit: confused single and double quotes

[ingydotnet]

This is resolved by #82.

The code for this PR is now in the run-test-suite branch and gets run by one
of:

• make -f .makefile test-all
• make test-suite
• ./tests/run-tests.sh (called by .travis.yml)