Hey all, I'd like review for: 6740590 Comments check can miss junk preceding bug synopsis
Webrev: http://cr.opensolaris.org/~richlowe/6740590 There are two webrevs in there, one against onnv, one against tooltest. I'd like people to look into this very carefully, as errors causing this to become overly strict can prevent integration, input from people who make such decisions appreciated. You'll notice that I removed an import of 'sys' on the tooltest side, this was found with pylint and pep8 while cleaning up some indent issues. Evaluation: The comment checks do not suitably anchor the bug synopsis match, so any comment ending in a valid synopsis and optional (fix stuff) comment is seen as valid, regardless of any preceding junk. We saw this recently with: changeset: 7389:7fc3f197d1fc user: Randall Ralphs <Randall.Ralphs at Sun.COM> date: Fri Aug 22 10:52:16 2008 -0600 summary: 6739108 Updated P2 kernel/io-multipath panic: (path_instance == mdi_pi_get_path_instance(vpkt-)vpkt_path)), file: ../../common/io/scsi/adap Which is erroneously seen as valid, despite being the subject line from mail from bugster, and thus not strictly of the correct format. Everything after the cat/subcat is the valid synopsis, and as such we saw it as valid in spite of the leading text. The fix is to anchor the match that we use there. ARC matching is not vulnerable to this, as we do straight string comparison, and deal with (fix stuff) separately (to account for the ARC case limit being 40 characters, and many cases going beyond that in their actual case naming) Testing: As updated tooltest webrev, running the tooltest against my workspace passes with no issues, running the updated tests against the current onbld fails in the appropriate places. I have seen one phantom error in the ARC case tests, which I can no longer reproduce, and may have been transient issues with opensolaris.org, I saw the issue once, and only once, and have been unable to repeat it in numerous runs. Either way, it seems likely to be unrelated to this change. My workspace: % python scmtest/src/legacy/scm/tooltest/tooltest.py -m usr/src/tools/ ................................................. ---------------------------------------------------------------------- Ran 49 tests in 8.782s OK Current bits: % python scmtest/src/legacy/scm/tooltest/tooltest.py ..........................F...................... ====================================================================== FAIL: comchk of valid bug/synopsis with junk leading/trailing text ---------------------------------------------------------------------- Traceback (most recent call last): File "scmtest/src/legacy/scm/tooltest/tests/test_comments.py", line 136, in testValidWithJunk AssertionError: '' != "These bugs/ARC case synopsis/names don't match the database entries:\nSynopsis/name of 5051903 is wrong:\n should be: 'Ypserv memory leak'\n is: 'foobar Ypserv memory leak'\n" ---------------------------------------------------------------------- Ran 49 tests in 7.723s FAILED (failures=1) You may notice that the tests within TestValidWithJunk are duplicative, we have a test for leading junk, one for trailing junk, and we also test with the precise bug and synopsis that brought this issue to my attention. This is intentional (if perhaps paranoia on my part) pbchk: onnv: no issues (hg pbchk -q is utterly silent) tooltest: as above. Thanks, -- Rich
