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

Move yaml-test-suite integration onto a separate branch. #82

Merged
merged 1 commit into from
Dec 30, 2017

Conversation

ingydotnet
Copy link
Member

@ingydotnet ingydotnet commented Dec 29, 2017

The logic for using the yaml-test-suite from libyaml master has been moved out
of master and onto its own branch called run-test-suite. The make test-suite command now checks out that branch before running the tests.

The tests run exactly the same as before but any changes to the way we use/run
the tests or changes to the testing manifest lists (which need to be kept up to
date with ongoing changes to the yaml-test-suite) no longer require a commit to
the master branch history.

This allows us to have lots of developmental activity to the tests without
disrupting master. Since a lot of things repackage libyaml, the master activity
will only be for actual bugfixes and enhancements.

The test logic (including @perlpunk's recent #81 work) is already on the
run-test-suite branch.

This should be the last commit we'll ever need on master wrt the
yaml-test-suite.

@perlpunk has noted that we want to pin the libyaml/test-lists/test-suite-data
commits so that we can test older versions of libyaml with the tests they were
meant to run against. This is easily done on the run-test-suite branch and
doesn't involve this PR to master at all.

@sigmavirus24
Copy link
Contributor

I don't think we should be using branches this way. Testing (metadata) is part of the code. I think it should live in master. I don't think evolving testing on master is a problem at all.

we want to pin the libyaml/test-lists/test-suite-data
commits so that we can test older versions of libyaml with the tests they were
meant to run against.

So we're going to be committing to master anyway to update pins, right? I don't think there's a significant difference between updating testing in master, or updating the SHA on another repository (or less ideally, branch) just to have fewer master commits related to testing.

Can you explain more of the motivation behind this change?

@ingydotnet
Copy link
Member Author

@sigmavirus24,

The yaml-test-suite is just one way to test libyaml and it really is not concerned with the existence of libyaml. I expect 1000s of tests for all kinds of concerns to be added to the yaml-test-suite in the near future. Our master branch should not need to change just because the test-suite changes or the way we are using it changes.

I want the master branch to be a low traffic history, that causes various downstreams to only react for important changes. All I'm doing here is separating the concerns. Everything will test as before; it already does on travis.

The planned pinning requires no changes to master. I'll add the code to the run-test-suite branch later this weekend, but the idea is that there is a pinning file (not in master) that has a line for each pin. When a new pin is needed you add a line for it. The test runner checks the HEAD commit of the code being tested and does the right thing according to the pinning file.

Finally, I would ask that we do it this way for now, since it makes no changes to master. If later we decide we should keep this in master we can do that.

@sigmavirus24
Copy link
Contributor

Our master branch should not need to change just because the test-suite changes or the way we are using it changes.

I don't entirely agree but it sounds like there's going to be a lot of volatility in the test suite that we probably don't want to cause problems when we fix issues. At the moment our test suite seems unreliable. Will this change help with that at all?

@ingydotnet
Copy link
Member Author

@sigmavirus24, I can't guess what you disagree with and I can't be sure of what you consider unreliable, but...

  1. No testing capabilities are lost here
  2. We now have @perlpunk's Add libyaml-parser-error.t to test invalid YAML #81 error-check testing working in travis: https://travis-ci.org/yaml/libyaml/jobs/323039897#L1019 (along with all the other whitelisted tests)
  3. We don't currently have any pinning capability, but I'll add this tonight if I can (it's simple)
  4. I suspect this will increase the reliability of libyaml testing greatly. :)

I personally think this is a win/win no-brainer. We get all the good things, and the ability to try new things in testing, all without disturbing the mainline dev history of a very important repo that we have taken over stewardship of.

PS. I'm on IRC if you want to chat.

@ingydotnet
Copy link
Member Author

@sigmavirus24,

I see now that you meant you didn't agree with my specific statement, "Our master branch should not need to change just because the test-suite changes or the way we are using it changes."...

Typically a test suite belongs to the project it is testing, and so I think it would belong in master. But this one is different. It's a big data pool for multiple spec versions and many concerns. Every YAML project will need a whitelist or blacklist (and probably more meta data) to determine how it uses it.

If libyaml were a new project of mine and/or if the yaml-test-suite was much more mature, I would probably keep that meta data in master. But neither are true at this time.

For now I want to get all the benefits from testing against this suite, without the risk of adding a lot of commits to libyaml master that I'll regret later. That's what this PR is giving us. We can switch it up later if/when it makes sense to.

@sigmavirus24
Copy link
Contributor

I can't be sure of what you consider unreliable

Lately, there seem to have been a number of changes to the test-suite lists that were necessary to make everything happy. If this introduces stability and helps us avoid that, at least on master, I'm in favor.

I can't guess what you disagree with

Sorry, I'm not at a computer, so I'm being unhelpfully brief. If we're going to gate changes on the test-suite, it seems like it makes the most sense for that to live in the branch. Further, I suspect this is just the maturity of the test-suite, so my concerns aren't a problem. I would like to see it eventually move back into master but I'm generally okay with this change.

@sigmavirus24
Copy link
Contributor

Hah, I just saw your newer comment. We're in violent agreement, although I don't mind the commits to master as much as you do :)

@ingydotnet
Copy link
Member Author

OK, I'm going to merge this for now. Thanks for making me explain myself more completely.

with the intent that `make test-suite` will use it as a branch.

This commit makes the libyaml and yaml-test-suite repos completely
independent. The only thing in master is a Makefile rule for
`make run-test-suite`.

The run-test-suite branch takes care of everything else.

This will keep the master branch history clean from any yaml-test-suite
activity, while keeping everything up to date.

Pinning will just work for whatever commit of master you run the tests
from. The pinning work will all be done in the `run-test-suite` branch.
@ingydotnet ingydotnet merged commit e4aee06 into master Dec 30, 2017
@ingydotnet ingydotnet deleted the branch-run-test-suite branch December 30, 2017 07:11
@perlpunk
Copy link
Member

Lately, there seem to have been a number of changes to the test-suite lists that were necessary to make everything happy.

There was one change required in libyaml-emitter.list because some of the out.yaml files in yaml-test-suite were corrected. The change was required because we don't pin yaml-test-suite yet. With pinning, the need for immediate changes goes away, with or without the change by this PR.

I have one concern now: the test/run-{parser,emitter}-test-suite.c files live in master. They output/process test suite events. The test scripts that use that files now live in the branch. I hope this will work even if we have to change the code.
I would have preferred to keep at least the test scripts in master. Changes could have been made in a normal branch.

Also, I would have preferred to see the pinning mechanism before merging this.

I wasn't aware that downstreams watch every commit. I thought only releases would matter.

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.

3 participants