Hi,
I'd like to extend elfutils testsuite, so I've examined lcov coverage report and picked up a function from libdw, dwarf_getstring(), which had coverage 0%, with this test it is 100% covered. The .c and .sh files are attached. Tested on x86_64. What do you folks think, is it sufficient? Any comments or suggestions are very welcome. Thanks,
Marek
tests/ChangeLog:
2011-03-02 Marek Polacek mpolacek@redhat.com
* dwarf-getstring.c: New test. * run-dwarf-getstring.sh: And its wrapper. * Makefile.am (EXTRA_DIST): Add and update all.
diff -d -urpN src/elfutils/tests/Makefile.am rpmbuild/BUILD/elfutils-0.152/tests/Makefile.am --- src/elfutils/tests/Makefile.am 2011-02-28 14:59:12.189848145 +0100 +++ rpmbuild/BUILD/elfutils-0.152/tests/Makefile.am 2011-03-02 20:56:30.089749019 +0100 @@ -57,7 +57,7 @@ noinst_PROGRAMS = arextract arsymtest ne dwfl-bug-addr-overflow arls dwfl-bug-fd-leak \ dwfl-addr-sect dwfl-bug-report early-offscn \ dwfl-bug-getmodules dwarf-getmacros addrcfi \ - test-flag-nobits + test-flag-nobits dwarf-getstring asm_TESTS = asm-tst1 asm-tst2 asm-tst3 asm-tst4 asm-tst5 \ asm-tst6 asm-tst7 asm-tst8 asm-tst9
@@ -83,7 +83,8 @@ TESTS = run-arextract.sh run-arsymtest.s run-dwfl-bug-offline-rel.sh run-dwfl-addr-sect.sh \ run-disasm-x86.sh run-disasm-x86-64.sh \ run-early-offscn.sh run-dwarf-getmacros.sh \ - run-test-flag-nobits.sh run-prelink-addr-test.sh + run-test-flag-nobits.sh run-prelink-addr-test.sh \ + run-dwarf-getstring.sh # run-show-ciefde.sh
if !STANDALONE @@ -117,6 +118,7 @@ EXTRA_DIST = run-arextract.sh run-arsymt run-addrname-test.sh run-dwfl-bug-offline-rel.sh \ run-dwfl-addr-sect.sh run-early-offscn.sh \ run-dwarf-getmacros.sh run-test-flag-nobits.sh \ + run-dwarf-getstring.sh \ testfile15.bz2 testfile15.debug.bz2 \ testfile16.bz2 testfile16.debug.bz2 \ testfile17.bz2 testfile17.debug.bz2 \ @@ -246,6 +248,7 @@ dwfl_bug_getmodules_LDADD = $(libdw) $(l dwfl_addr_sect_LDADD = $(libdw) $(libebl) $(libelf) $(libmudflap) -ldl sha1_tst_LDADD = $(libeu) $(libmudflap) dwarf_getmacros_LDADD = $(libdw) $(libmudflap) +dwarf_getstring_LDADD = $(libdw) $(libmudflap) addrcfi_LDADD = $(libdw) $(libebl) $(libelf) $(libmudflap) -ldl test_flag_nobits_LDADD = $(libelf) $(libmudflap)
The normal way to send code is as a single patch, except for the ChangeLog entries (which should be prepended just like you did it). The new files should be part of the patch, and it should be done so that "patch -p1" or "patch -p0" applies it correctly (or ideally, "git apply --index"). The easiest thing to do is to do your work on a local git branch and then use "git diff" or "git format-patch" to make the patch.
Really the best thing to do is to join the "gitelfutils" group via the https://admin.fedoraproject.org/accounts system. Once you're approved there, you can push to git directly. The convention is that you are free to push any branches you like that are called <username>/* where <username> is your fedoraproject.org user name (and * is anything, of course). If you push a branch like that with a new change, then it is very easy for us to merge it in.
The dwarf_getstring function is not one that really ever gets used, so there isn't a lot of need for this. But anything improving the test coverage is a good thing, and that test is just fine as it's written. It's a fine place to start for a first contribution.
Thanks, Roland
On Wed, 2011-03-02 at 12:33 -0800, Roland McGrath wrote:
Really the best thing to do is to join the "gitelfutils" group via the https://admin.fedoraproject.org/accounts system. Once you're approved there, you can push to git directly. The convention is that you are free to push any branches you like that are called <username>/* where <username> is your fedoraproject.org user name (and * is anything, of course). If you push a branch like that with a new change, then it is very easy for us to merge it in.
Done. Thanks for approving. I've created mpolacek/coverage branch. The new tests altogether with changed ChangeLog and Makefile.am are already there. Now when I think over, I probably should've made only one commit for that, not three. But I hope it can be merged now.
The dwarf_getstring function is not one that really ever gets used, so there isn't a lot of need for this. But anything improving the test coverage is a good thing, and that test is just fine as it's written. It's a fine place to start for a first contribution.
I was concerned about that. In libdw there are a few other functions like that. I don't know if it would be better to write just one test for all of them or write a set of tests each testing one particular function. I don't want to pollute tests/ directory so I'd favor the first option. Suggestions? Thank you,
Marek
Done. Thanks for approving. I've created mpolacek/coverage branch. The new tests altogether with changed ChangeLog and Makefile.am are already there. Now when I think over, I probably should've made only one commit for that, not three. But I hope it can be merged now.
Yes, those should all be in one commit. I used 'git merge --squash' to turn them into one. That isn't the same as a real merge. So you should now do 'git push origin :mpolacek/coverage' to remove your old branch rather than merging master into it. In general, it's a better idea to use a more-specific branch name for something you are submitting to be merged. That way, if you have more test coverage changes to add, they will go on separate branches that we can review independently and there won't be any undesireable history in the branches we want to merge in case one change is ready for merging but another needs to be reworked.
I was concerned about that. In libdw there are a few other functions like that. I don't know if it would be better to write just one test for all of them or write a set of tests each testing one particular function. I don't want to pollute tests/ directory so I'd favor the first option. Suggestions?
It's not really a big deal either way. In general, smaller tests are better because if there is something to be debugged, it isolates the problem into a simple case that's all one has to look at. The purpose of the tests directory is to have tests, so adding more tests there is not pollution.
Thanks, Roland
elfutils-devel@lists.fedorahosted.org