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

Data-driven tests #5098

Merged
merged 2 commits into from
Jun 11, 2019
Merged

Data-driven tests #5098

merged 2 commits into from
Jun 11, 2019

Conversation

pks-t
Copy link
Member

@pks-t pks-t commented Jun 7, 2019

tests: allow for simple data-driven tests

Right now, we're not able to use data-driven tests at all. E.g.
given a set of tests which we'd like to repeat with different
test data, one has to hand-write any outer loop that iterates
over the test data and then call each of the test functions. Next
to being bothersome, this also has the downside that error
reporting is quite lacking, as one never knows which test data
actually produced failures.

So let's implement the ability to re-run complete test modules
with changing test data. To retain backwards compatibility and
enable trivial addition of new runs with changed test data, we
simply extend clar's generate.py. Instead of scanning for a
single _initialize function, each test module may now implement
multiple _initialize_foo functions. The generated test harness
will then run all test functions for each of the provided
initializer functions, making it possible to provide different
test data inside of each of the initializer functions. Example:

void test_git_example__initialize_with_nonbare_repo(void)
{
        g_repo = cl_git_sandbox_init("testrepo");
}

void test_git_example__initialize_with_bare_repo(void)
{
        g_repo = cl_git_sandbox_init("testrepo.git");
}

void test_git_example__cleanup(void)
{
        cl_git_sandbox_cleanup();
}

void test_git_example__test1(void)
{
        test1();
}

void test_git_example__test2(void)
{
        test2();
}

Executing this test module will cause the following output:

$ ./libgit2_clar -sgit::example
git::example (with nonbare repo)..
git::example (with bare repo)..

The goal was basically to have the least-intrusive change that gives us the most bang for the buck. No changes were required for any of our existing test modules, but we can now easily set up multiple test runs of a complete module by just specifying multiple initializers.

pks-t added 2 commits June 7, 2019 14:23
Right now, we're not able to use data-driven tests at all. E.g.
given a set of tests which we'd like to repeat with different
test data, one has to hand-write any outer loop that iterates
over the test data and then call each of the test functions. Next
to being bothersome, this also has the downside that error
reporting is quite lacking, as one never knows which test data
actually produced failures.

So let's implement the ability to re-run complete test modules
with changing test data. To retain backwards compatibility and
enable trivial addition of new runs with changed test data, we
simply extend clar's generate.py. Instead of scanning for a
single `_initialize` function, each test module may now implement
multiple `_initialize_foo` functions. The generated test harness
will then run all test functions for each of the provided
initializer functions, making it possible to provide different
test data inside of each of the initializer functions. Example:

```
void test_git_example__initialize_with_nonbare_repo(void)
{
	g_repo = cl_git_sandbox_init("testrepo");
}

void test_git_example__initialize_with_bare_repo(void)
{
	g_repo = cl_git_sandbox_init("testrepo.git");
}

void test_git_example__cleanup(void)
{
	cl_git_sandbox_cleanup();
}

void test_git_example__test1(void)
{
	test1();
}

void test_git_example__test2(void)
{
	test2();
}
```

Executing this test module will cause the following output:

```
$ ./libgit2_clar -sgit::example
git::example (with nonbare repo)..
git::example (with bare repo)..
```
The object::cache test module has two tests that do nearly the
same thing: given a cache limit, load a certain set of objects
and verify if those objects have been cached or not.

Convert those tests to the new data-driven initializers to
demonstrate how these are to be used. Furthermore, add some
additional test data. This conversion is mainly done to show this
new facility.
@ethomson
Copy link
Member

Well, isn't this clever. I'm just driving by at the moment, so I'll need to put a little more 👀 on the implementation but I love the idea.

Hey @vmg, we've been making a lot of changes (okay, not a lot, but also not zero) to clar lately. Do you want us to try to upstream these back to you, or...?

@vmg
Copy link
Member

vmg commented Jun 10, 2019

Nah, clearly the version here in libgit2 is the "definitive" version of Clar. I may just delete my repo and point people here. You're way ahead. 😅

@ethomson
Copy link
Member

Nah, clearly the version here in libgit2 is the "definitive" version of Clar. I may just delete my repo and point people here. You're way ahead. 😅

I like that it's broken out into a separate repo though (I use it in other projects as well). 🤔

@vmg
Copy link
Member

vmg commented Jun 10, 2019

Could we place it in a separate folder here in libgit2 so it's easier to copy&paste into other projects?

@ethomson
Copy link
Member

If you're anxious to get rid of https://github.com/vmg/clar then we can either fork that over to the @libgit2 account (or to one of our personal accounts) and make that the new canonical place. Don't know if anybody else has a strong preference.

I lean away from @libgit2 a little bit, because I don't want anybody to think that the Team As A Whole supports it.

@tiennou
Copy link
Contributor

tiennou commented Jun 10, 2019

Same here, I'd really like clar to keep a "canonical" home, outside of any other project, whether it's as part of the org or where it still lives (no strong opinion). I've tried once to try to resync with upstream via a subtree merge, but it didn't end well (IIRC, there were conflicting/incomplete fixes for a bunch of the same issues in both "ours" and "theirs", plus the XML report generation).

It might be possible to import it as a git submodule instead ? Not sure how we feel about that though, but it might be better located as tests/deps{,/clar} or something, so tracking "upstream" is easier.

I've started to use it as the test harness in libssh2, and I'm already carrying some custom stuff there (more vararg-y macros, so tests can build more complex messages). So I'm fine to be added to some @libgit2/clar team, if we feel one is needed.

Copy link
Contributor

@tiennou tiennou left a comment

Choose a reason for hiding this comment

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

I like it, it's a straightforward solution. I feel like a few more converted testcases would look even better 😉.

@pks-t
Copy link
Member Author

pks-t commented Jun 11, 2019

@tiennou: happy to convert more in case you know of some where it would make sense to do so! Merge analysis in #5017 is an obvious next one, but that's not merged yet.

If you're anxious to get rid of https://github.com/vmg/clar then we can either fork that over to the @libgit2 account (or to one of our personal accounts) and make that the new canonical place. Don't know if anybody else has a strong preference.

I don't. I feel like I'm usually the one driving for new features in clar for the libgit2 project, so I might as well fork it into my own personal namespace. But in general, I always prefer the more "official" way of using an org for that. "clar" is taken already, but we may opt for something like "clar-tests". I'd also prefer the second approach in order to not become the sole maintainer, but instead to have a team of maintainers like in libgit2.

Could we place it in a separate folder here in libgit2 so it's easier to copy&paste into other projects?

I agree that we should probably refactor this. Currently, we're quite inconsistent with regards to third-party dependencies, where we have some in "deps/" and some mixed into our own sources. If we agree to do that, then I might clean it up in the near-term future.

@tiennou
Copy link
Contributor

tiennou commented Jun 11, 2019

happy to convert more in case you know of some where it would make sense to do so!

I was thinking about the worktree/bare/non-bare. Arguably, I'm looking very hard at most of repo, because running that using a bunch of normal, bare, shallow, in-memory, mostly-memory, repositories would make the most sense, but it's harder (though maybe taking a look at "how" would benefit the proposed implementation).

@pks-t
Copy link
Member Author

pks-t commented Jun 11, 2019 via email

@ethomson
Copy link
Member

I agree, but I think we should keep it out of this PR.

👍

This is neat, I like it a lot. Thanks @pks-t.

@ethomson ethomson merged commit ff7652c into libgit2:master Jun 11, 2019
@pks-t pks-t deleted the pks/clar-data-driven branch June 13, 2019 09:06
@pks-t
Copy link
Member Author

pks-t commented Jun 13, 2019

Thanks for merging, @ethomson! We still didn't agree on how to fare with clar, though. Should we maintain it as a separate project "clar-{framework,tests,testing}/clar" (proposals welcome!) and then re-synchronize our own copy with such an upstream project?

@tiennou
Copy link
Contributor

tiennou commented Jun 13, 2019

I've just filed clar-test/clar#81, which carries our current changes.

@pks-t
Copy link
Member Author

pks-t commented Jun 13, 2019

Cool, thanks for your effort! If @vmg wants to keep maintaing clar then that's probably the best way ahead. To me it sounded like he didn't, though, but it might be that I interpreted too much into his words. :)

@ethomson
Copy link
Member

Sounds like @vmg would like to create an org for this, so I've created https://github.com/clar-test/clar and invited all of you. @vmg do you want to do that magic to make that one the root of the network? I forget how that works.

@vmg
Copy link
Member

vmg commented Jun 17, 2019

@ethomson I think I need admin access to the clar repo in the new org before I can do that!

@ethomson
Copy link
Member

ethomson commented Jun 17, 2019 via email

@ethomson
Copy link
Member

ethomson commented Jul 1, 2019

@vmg I've also deleted https://github.com/clar-test/clar, so you're welcome to do that transfer if you'd like.

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.

None yet

4 participants