I'd like to know what you think. The commit message would be:
Add a regression test mechanism along with tests for most of recent changes.
This was copied from David Holland's tradcpp and modified to also test switches.
Details
Details
- Reviewers
pfg ngie - Commits
- rS313544: indent(1): add regression test cases
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Previous version was incompatible with make -j. Use option -P from D9010 to load the profile file directly instead of doing the cp/rm dance.
Comment Actions
I am glad you are passing this through phabricator. I don't have much experience with the testing framework. Let me add the local expert.
Comment Actions
This doesn't integrate the tests into the test framework.
Some of the how for doing this is documented in https://wiki.freebsd.org/TestSuite , share/mk/bsd.README , and atf-test-case(7) .
There are a couple different examples to go off of -- bin/date/tests/format_string_test.sh is a good one , bin/ls/tests/ls_tests.sh is a more fully fledged test script. Please also look at the related Makefiles -- they're a lot simpler
As far as a fully fledged "I want to commit all the pieces I need to integrate my tests into the FreeBSD test suite" commit is concerned, please take a look at: https://svnweb.freebsd.org/base?view=revision&revision=284388 .
Don't hesitate to ask questions!
| tests/Makefile | ||
|---|---|---|
| 1 ↗ | (On Diff #23490) | INDENT_OBJDIR=<tab>${.OBJDIR:H} |
| 4–5 ↗ | (On Diff #23490) | Please use the following format: TESTS+=<tab>comments TESTS+=<tab>declarations # ... It makes it easier to add more tests and backport them in the future without conflicts. |
| tests/declarations.good | ||
| 6–7 ↗ | (On Diff #23490) | This looks incorrect per style(9). |
| 10–13 ↗ | (On Diff #23490) | This looks incorrect per style(9). |
| tests/elsecomment.good | ||
| 5–8 ↗ | (On Diff #23490) | This looks incorrect per style(9). |
| tests/list_head.good | ||
| 13 ↗ | (On Diff #23490) | This looks incorrect per style(9). |
| tests/offsetof.good | ||
| 5 ↗ | (On Diff #23490) | This looks a bit excessive per style(9). |
| tests/struct.good | ||
| 8 ↗ | (On Diff #23490) | This looks a bit excessive per style(9). |
| 9–10 ↗ | (On Diff #23490) | It looks like spaces are missing after "{" and "}". |
Comment Actions
Thanks for the review: we do have old tests lying around that don't integrate with ATF. I guess it's sensible to expect that all new test do so. Thanks for the pointers.
FWIW, about style(9), the tests are only meant to see if indent(1) does what is asked to do. Since this is not code that is expected to run it is OK (I think).
Comment Actions
This looks incorrect per style(9).
As Pedro said, the tests are meant to check if indent(1) works as expected, not that it always produces style(9)-conformant formatting. We'll get closer to that, later, but I need a regression tests suite first.
This doesn't integrate the tests into the test framework.
Some of the how for doing this is documented in https://wiki.freebsd.org/TestSuite , share/mk/bsd.README , and atf-test-case(7) .
There are a couple different examples to go off of -- bin/date/tests/format_string_test.sh is a good one , bin/ls/tests/ls_tests.sh is a more fully fledged test script. Please also look at the related Makefiles -- they're a lot simpler
As far as a fully fledged "I want to commit all the pieces I need to integrate my tests into the FreeBSD test suite" commit is concerned, please take a look at: https://svnweb.freebsd.org/base?view=revision&revision=284388 .
That's likely a good idea for the future. Currently I only need as much as the proposed patch does in order to continue my work on indent(1). Perhaps someone else could join in and integrate these tests into the test framework.
Thank you for your time, effort and valuable advice.
| tests/Makefile | ||
|---|---|---|
| 1 ↗ | (On Diff #23490) | Thanks, will do. Could you tell me why ${.OBJDIR:H} is better? |
| 4–5 ↗ | (On Diff #23490) | I agree, but it would make transferring commits from my github repository to svn a bit painful. Perhaps I could commit as-is and use the preferred format in a later commit? |
| 16 ↗ | (On Diff #23490) | I'll drop the .if exists(). -P will silently ignore the fact that the profile file doesn't exist. |
| tests/struct.good | ||
| 9–10 ↗ | (On Diff #23490) | bde also pointed it out. indent(1) has many bugs, I haven't fixed this one yet. I'd like to have a regression test suite first. |
Comment Actions
Fix details in the Makefile pointed out by ngie,
In greatest disgust, add license headers to all test case files, per suggestion from rwatson and cpercival.
Rebased on top of SVN.
Comment Actions
Having a license is important and the committer guide specifies a two clause BSD license as the preferred. for new files In this case the license uses more space that the tests though, so perhaps it is worth discussing in the lists the possibility of using a shorter statement like CC0. IANAL ... so I generally avoid such discussions.
Still missing, you need a line like this:
/* $FreeBSD$ */
(There is a commit hook that will prevent you from committing without it)
I prefer it after the license, some people put it as the first line.
ngie is listed as MAINTAINER for tests, so in this case approval by ngie means I approve it too.
Comment Actions
OK, I see that Piotr has followed the suggestions, so I'll reformulate the implicit question. Are there any objections against committing this?
Comment Actions
These tests are still not integrated into the FreeBSD test suite. This is a blocker from committing the change to head.
Comment Actions
The patch adds value at no cost. Why would lack of FreeBSD test suite integration be a blocker? That can be added later.
Comment Actions
There are some costs:
- In order for someone else to use your tests, they have to figure out how things work and decipher the results.
- There will be future commits in order to integrate the tests, which will involve more MFCs and approvals from your mentor.
- It not a good example for others trying to figure out how things work.
Comment Actions
...
There are some costs:
- In order for someone else to use your tests, they have to figure out how things work and decipher the results.
- There will be future commits in order to integrate the tests, which will involve more MFCs and approvals from your mentor.
- It not a good example for others trying to figure out how things work.
Put differently, I cannot approve this because it doesn't really integrate into the existing system I contribute to (the FreeBSD Test Suite). If pfg@ approved the commit, it's fine to commit as-is.
Comment Actions
If you intend to work on this further, please put a disclaimer in the commit, tack on an appropriate MFC date, and I'll approve that just from a review perspective, but I don't doubt that you'll get some comments from various parties (other than me) because your make snippets don't integrate into bsd.*.mk and it doesn't integrate into ATF/kyua..
Comment Actions
I am personally not used to the testsuite, but I recall we do have tests that are not fully integrated into kyua: as examples libedit, sed and printf(1). I will also state I don't like our testsuite, partly because I don't know it well, but it's what we have.
I haven't approved it. I like the idea of having a test suite for indent(1) but I don't see the hurry to put it into head unless it's acceptable to the FreeBSD testsuite guys. I do appreciate Piotr's work on this but the thing about WIP is that it generally stays as WIP.
Comment Actions
There is no hurry. But I would like to commit the regression tests before committing any further fixes and new features since most of them would be committed together with new code under tests/ that would demonstrate what the commit is about. See this for an example: https://github.com/pstef/freebsd_indent/commit/e94696ba10904636556dc9d4d498b836e67a06cb
I do appreciate Piotr's work on this but the thing about WIP is that it generally stays as WIP.
From one perspective, everything I do and commit is a work in progress. From another perspective, this patch is pretty self-contained and easily can be viewed as an intermediary step between no regression tests and regression tests integrated with the FreeBSD Test Suite.
This is the value added, not the cost. Currently, that someone would have to write their own tests. And then the choice they would have would be either keep it for personal use or integrate them into the FreeBSD test suite in order to share with all the others.
If you intend to work on this further, please put a disclaimer in the commit, tack on an appropriate MFC date, and I'll approve that just from a review perspective, but I don't doubt that you'll get some comments from various parties (other than me) because your make snippets don't integrate into bsd.*.mk and it doesn't integrate into ATF/kyua..
What do you mean by "your make snippets don't integrate into bsd.*.mk"?
Comment Actions
...
There is no hurry. But I would like to commit the regression tests before committing any further fixes and new features since most of them would be committed together with new code under tests/ that would demonstrate what the commit is about. See this for an example: https://github.com/pstef/freebsd_indent/commit/e94696ba10904636556dc9d4d498b836e67a06cb
This CR just delays the integration work though.
I do appreciate Piotr's work on this but the thing about WIP is that it generally stays as WIP.
From one perspective, everything I do and commit is a work in progress. From another perspective, this patch is pretty self-contained and easily can be viewed as an intermediary step between no regression tests and regression tests integrated with the FreeBSD Test Suite.
This is kind of splitting hairs.
Yes, I know that this is a WIP, but the WIP I'm referring to is the fact that there's test input and output being committed to the tree which is not really digestible, except to you (I'm kind of tired of having to integrate others' tests into the framework -- I did a lot of that with test code under tools/regression/ :(..).
...
- In order for someone else to use your tests, they have to figure out how things work and decipher the results.
This is the value added, not the cost. Currently, that someone would have to write their own tests. And then the choice they would have would be either keep it for personal use or integrate them into the FreeBSD test suite in order to share with all the others.
Not really. I have no idea how to execute your tests unless I cd to that directory, open the Makefile, and try and understand the Makefile, compile a copy of indent, then run your target.
If you intend to work on this further, please put a disclaimer in the commit, tack on an appropriate MFC date, and I'll approve that just from a review perspective, but I don't doubt that you'll get some comments from various parties (other than me) because your make snippets don't integrate into bsd.*.mk and it doesn't integrate into ATF/kyua..
What do you mean by "your make snippets don't integrate into bsd.*.mk"?
Read share/mk/bsd.README . You are inventing a bunch of targets you don't need to invent because you're not leveraging existing infrastructure.
Comment Actions
I have a potential compromise: please remove the "Makefile" from the commit, integrate your tests into the FreeBSD test suite separately, open another CR for that change, and I'll approve that change once it's complete.
Comment Actions
Do you mean it'd be OK to commit the test cases (*.c, *.good, *.pro) without the Makefile? I'd be fine with that.
Comment Actions
This doesn't make much sense to me but as I said, if ngie@ approves it I am OK with it too. Please do state in the commit log that the tests are currently disconnected from the build but that you intend to complete them. As before, rember to include a link to the CR.
Comment Actions
I can help guide piotr with the integration work, once the test content is present.
One thing that would be good (BTW) is to not use dashes in filenames (use underscores instead?) to make the direct translation between test files and testcases more seamless.
I recommend a commit message like the following:
Add initial tests for indent <add more reasoning here?> The tests are disconnected from the build, but will be integrated into the FreeBSD test suite eventually. Approved by:<tab><tab>pfg (mentor) Reviewed by:<tab><tab>ngie MFC after:<tab><tab>1 month Differential Revision:<tab>https://reviews.freebsd.org/D9007
Comment Actions
OK, that sounds good.
One thing that would be good (BTW) is to not use dashes in filenames (use underscores instead?) to make the direct translation between test files and testcases more seamless.
I recommend a commit message like the following:
Add initial tests for indent <add more reasoning here?> The tests are disconnected from the build, but will be integrated into the FreeBSD test suite eventually. Approved by:<tab><tab>pfg (mentor) Reviewed by:<tab><tab>ngie MFC after:<tab><tab>1 month Differential Revision:<tab>https://reviews.freebsd.org/D9007
I am not sure about the MFC: There are 30-something changes in
head that have not been MFC'd (and I am just too lazy to merge them).
Comment Actions
:{..
Code should be MFCed though if possible -- otherwise you won't get feedback on your commits until the next .0 release cycle (which will be a couple years from now).
I think it'd be good practice for piotr to do -- it helps reinforce good habits.
Comment Actions
...
I am not sure about the MFC: There are 30-something changes in
head that have not been MFC'd (and I am just too lazy to merge them).:{..
Code should be MFCed though if possible -- otherwise you won't get feedback on your commits until the next .0 release cycle (which will be a couple years from now).
Currently keeping the older indent untouched is useful to detect regressions. Bruce Evans pointed some already.
I think it'd be good practice for piotr to do -- it helps reinforce good habits.
When indent(1) is more stable (or even finished) we can consider it.
Comment Actions
Please do state in the commit log that the tests are currently disconnected from the build but that you intend to complete them. As before, rember to include a link to the CR.
I can help guide piotr with the integration work, once the test content is present.
I would like to leave that for someone who wants to improve the FreeBSD test suite. I don't want to spend time on learning it, I want to spend time on actually fixing things.
Comment Actions
-_-... this is what I was concerned about.
Ok, since I'll be doing the integrating (apparently), I need you to follow a specific scheme for the filenames (standard error, standard out, input files) so I can have it automatically find all of the tests and act appropriately, like bin/sh/tests/functional_test.sh does.
Otherwise, I'm starting to doubt the value of what you're committing, even if it lacks the Makefile because it will be an annoying maintenance burden for me and other folks who do work with the test suite.
This comment was removed by pstef.
Comment Actions
What is the specific scheme? I tried to figure it out of functional_test.sh but it's too complicated for me.
Otherwise, I'm starting to doubt the value of what you're committing, even if it lacks the Makefile because it will be an annoying maintenance burden for me and other folks who do work with the test suite.
Comment Actions
After learning about PR 215690 I'm no longer convinced that the regression tests I'd like to have should be integrated into the FreeBSD testing suite. It sounds ridiculous that a harmless regression in a reformatting tool like indent(1) would create "bogus failures counted against our overall results". indent(1) is not a crucial part of the operating system and shouldn't be tested as rigorously. Bugs in indent(1) are not even close to an indication of overall quality of the system.
At this point I'm leaning towards submitting this humble regression test suite for inclusion in NetBSD; then I could fix indent(1) there and simultaneously port real changes back to FreeBSD, avoiding this "regression test suite integration" obstacle.
Comment Actions
Well, I've been arguing all along that perhaps the tests don't belong in FreeBSD's tree but that you could keep them in github with travis-ci. Regression tests are generally meant to make sure other commits don't break something that you have fixed: it doesn't have to be something critical, but simply something that we could consider a bug. Given the effort to just get the tests in, I would suggest leaving that step for last in the development cycle.
At this point I'm leaning towards submitting this humble regression test suite for inclusion in NetBSD; then I could fix indent(1) there and simultaneously port real changes back to FreeBSD, avoiding this "regression test suite integration" obstacle.
I think that could work from a more practical standpoint. NetBSD's test framework is upstream for ours: if they take your testsuite and integrate it, between NetBSD and ngie will do the required adaptions. I have no idea how long it takes to upstream such changes in NetBSD though and it does seem like an unnecessary walk, but I don't have an easy solution.
Comment Actions
I see two drawbacks of that:
- dependency on an external mechanism,
- disconnection between the changes and the examples showing what the change is about.
Given the effort to just get the tests in, I would suggest leaving that step for last in the development cycle.
This has the same drawback (2) as the above.
At this point I'm leaning towards submitting this humble regression test suite for inclusion in NetBSD; then I could fix indent(1) there and simultaneously port real changes back to FreeBSD, avoiding this "regression test suite integration" obstacle.
I think that could work from a more practical standpoint. NetBSD's test framework is upstream for ours: if they take your testsuite and integrate it, between NetBSD and ngie will do the required adaptions. I have no idea how long it takes to upstream such changes in NetBSD though and it does seem like an unnecessary walk, but I don't have an easy solution.
David Holland is kind of waiting for me to port the fixes to their copy of indent(1) which is only slightly different from ours. He personally encouraged me to take tests/Makefile from his own tradcpp and even reviewed my additions to that mechanism. He's also discouraged me from using ATF/Kyua for that and now I can perfectly see why. So I imagine it would be fairly straightforward to get it pushed there, but I don't really know.
Comment Actions
Hard stop please
First off, bug 215690 should not be a reason for not integrating the tests into ATF/Kyua. That's a really bad straw man:
- It works for over 6000 testcases in FreeBSD already.
- cem's testcase was written for ATF/Kyua and not properly tested before it was committed to FreeBSD (which irritates me greatly because it foists the work of getting it to the finish line to me/jmmv before the enhancement needed for atf/kyua is made upstream).
We're spending too much time debating minutia. Since it looks like I'm going to be doing the work of integrating things into ATF/Kyua, I request:
- A consistent file naming scheme for inputs/outputs and a way to determine whether or not the output passed/failed if a negative test, like what /bin/sh/tests does in naming the testcases [canonically] like "<testcase>.<exit-code>" and name their stdout/stderr files like "<testcase-canonical>.stdout" and "<testcase-canonical>.stderr". What doing this does is allows me to create a test driver _once_, and every time you add a new testcase, all you need to do is add the inputs/outputs to ${PACKAGE}FILES in the Makefile, thus making your job braindead easy to do.
- Please don't use "-" in filenames -- otherwise I have to do some shell gymnastics when dynamically creating testcases in the test driver.
As far as getting the code back to NetBSD is concerned, I've been pushing a lot of changes back to them via http://www.netbsd.org/cgi-bin/sendpr.cgi?gndb=netbsd after I test out my changes on an amd64 NetBSD CURRENT VM. Once the work is done here, either you or I can push it back up to the NetBSD folks, and I can suck the changes in the next time I update contrib/netbsd-tests .
Please don't manually add the tests to contrib/netbsd-tests without adding me on a CR (otherwise I will miss changes that need to be handled upstream -- like a handful of edits that FreeBSD devs have done in the past few years that caught me by surprise recently when I tried reconciling all of our diffs).
Comment Actions
Much better.
Could your commit message please describe what each of the files does, e.g.
.stdout - standard output
.pro (why .pro BTW?) - arguments list
.list - ?
etc?
It would also be good if the .pro and .list files had the exit code in them so all I needed to do is call that good as the prefix.
Thanks!
-Ngie
| types_from_file.pro | ||
|---|---|---|
| 1 ↗ | (On Diff #24260) | Typo? |
Comment Actions
What the description does is provides me an idea of what I need to code up for a test driver and also gives anyone (who executes the tests by hand) an idea of what the inputs and outputs are, so even if the test driver wasn't present, the tests could be run manually.
Comment Actions
I forgot to update a file that referred to the file name where I replaced hyphens with underscores.
Comment Actions
Sure.
.stdout - standard output
.pro (why .pro BTW?) - arguments list
indent has called the profile file "indent.pro" for ages; I only added the ability to specify your own file name. I don't see a good reason to change the extension now.
.list - ?
It's a list of non-standard types. I don't think it should be described in the commit message as it's a file connected with the types_from_file tests and not connected with anything else.
etc?
It would also be good if the .pro and .list files had the exit code in them so all I needed to do is call that good as the prefix.
Why? Profile files (.pro) are neither input nor output. Any automatic regression mechanism should in essence do this:
indent ${test}.0 ${test}.0.run -P${test}.proand compare ${test}.0.stdout with ${test}.0.run. (Both .0 and .0.run are files. BSD indent(1) has never been able to produce re-formatted input to stdout.)
Comment Actions
... except when input is from stdin. So this works:
indent -P${test}.pro < ${test}.0 > ${test}.0.runComment Actions
Sorry to bother you, but I have some doubts and I'd prefer to ask here first than to discuss on -committers when it's too late.
My main doubt is whether the expected output should be absolutely correct in every aspect (this won't happen until after 20 to 30 commits, I think) or can the output files be good enough now and perfected later?
Secondly, I'd like to replace some of the output with generic code showing the same problem. My concern here is potential copyright issues (although that'd be a weak case because of the non-generic examples there are only two sources: FreeBSD and Postgres). Doing this replacement would probably invalidate your positive review, no? I don't know how this works, yet.
Comment Actions
It's certainly better to ask early, no need to apologize.
My main doubt is whether the expected output should be absolutely correct in every aspect (this won't happen until after 20 to 30 commits, I think) or can the output files be good enough now and perfected later?
I think you can tag them as expected failures or otherwise your could leave them commented until it makes sense to have them run. Another way around that would be to commit the test gradually along with the fixes for each issue.
Secondly, I'd like to replace some of the output with generic code showing the same problem. My concern here is potential copyright issues (although that'd be a weak case because of the non-generic examples there are only two sources: FreeBSD and Postgres). Doing this replacement would probably invalidate your positive review, no? I don't know how this works, yet.
If you need to use the PostgreSQL license, that looks pretty MIT/BSD ish and shouldn't be a problem. Since that depends on a case per case basis it does need a different review.
Comment Actions
I think that the working tests should be committed incrementally. This will put a number of commits/MFCs under your belt, which will be helpful for completing your mentorship, and is good practice for backporting changes for indent.
Secondly, I'd like to replace some of the output with generic code showing the same problem. My concern here is potential copyright issues (although that'd be a weak case because of the non-generic examples there are only two sources: FreeBSD and Postgres). Doing this replacement would probably invalidate your positive review, no? I don't know how this works, yet.
I think stripped down examples would be best. If you're concerned about licensing, I'd reach out to core@ about this (it might cause some annoying alarm bells to be run with anyone that uses protoip blackduck for analyzing the copyrights in use). At the very least they should provide you "covering fire" if needed, and/or direct you to the Foundation's lawyer on retainer.
Comment Actions
I did plan to commit them incrementally in the sense that I would add a test case for a new feature together with the feature implementation. The question is rather: can these starting tests fail until I fix indent(1) so that they don't? Pedro pointed out that tests can be marked as "expected to fail", but I don't know anything about that.
Secondly, I'd like to replace some of the output with generic code showing the same problem. My concern here is potential copyright issues (although that'd be a weak case because of the non-generic examples there are only two sources: FreeBSD and Postgres). Doing this replacement would probably invalidate your positive review, no? I don't know how this works, yet.
I think stripped down examples would be best. If you're concerned about licensing, I'd reach out to core@ about this (it might cause some annoying alarm bells to be run with anyone that uses protoip blackduck for analyzing the copyrights in use). At the very least they should provide you "covering fire" if needed, and/or direct you to the Foundation's lawyer on retainer.
I've already decided to strip them down, I only meant that if I do that then what's about to be committed is not the same thing that you've approved.
Comment Actions
...
I did plan to commit them incrementally in the sense that I would add a test case for a new feature together with the feature implementation. The question is rather: can these starting tests fail until I fix indent(1) so that they don't? Pedro pointed out that tests can be marked as "expected to fail", but I don't know anything about that.
It would be a bit more complicated building in "expected failures" for the test driver, but it could be done. Is there any value to committing in the failures without the code being fixed (I mean, yes, I understand the practical reasoning... I just wasn't sure if committing things now and making the problems visible is any benefit to others, except when dealing with bus factor, i.e. the issues can be tracked)?
I've already decided to strip them down, I only meant that if I do that then what's about to be committed is not the same thing that you've approved.
If you update the output, I'll give you a quick ship-it. I'm mostly concerned about how things are going to integrate with your tests right now, not the test inputs/outputs as much -- you're the main stakeholder with the test content ;).
Comment Actions
I don't think there are any practical implications. I see it as choosing the lesser evil: either svn history will show that the idea of desired behavior has been changing over relatively short time span or you'll have failures in the testing suite. I can live with either.
I've already decided to strip them down, I only meant that if I do that then what's about to be committed is not the same thing that you've approved.
If you update the output, I'll give you a quick ship-it. I'm mostly concerned about how things are going to integrate with your tests right now, not the test inputs/outputs as much -- you're the main stakeholder with the test content ;).
OK
Comment Actions
Together with proposed commit message:
indent(1): add regression test cases
These examples show expected behavior of indent(1). They are meant to be used
together with a regression test mechanism, either Kyua, a Makefile or perhaps
something else. The mechanism should in essence do this:
indent -P${test}.pro < ${test}.0 > ${test}.0.runand compare ${test}.0.stdout to ${test}.0.run. If the files differ or the
exit status isn't 0, the test failed.
${test}.pro is an indent(1) profile: a list of options passed through a file.
The program doesn't complain if the file doesn't exist.
${test}.0 is a C source file which acts as input for indent(1). It doesn't
have to have any particular formatting, since it's the output that matters.
${test}.0.stdout contains expected output. It doesn't have to be formatted in
Kernel Normal Form as the point of the tests is to check for regressions in
the program and not to check that it always produces KNF.
Reviewed by: ngie
Approved by: pfg (mentor)
Differential Revision: https://reviews.freebsd.org/D9007
Comment Actions
I'd add bullets to the commit message for readability. Otherwise, looks wonderful -- thanks :)!