thanks for looking at this. the webrev is updated. replies inline below. ed
On Thu, Sep 06, 2012 at 11:58:36AM -0700, Danek Duvall wrote: > Edward Pilatowicz wrote: > > > https://cr.opensolaris.org/action/browse/pkg/edp/pkg.change-facet/webrev > > imageplan.py: > > - line 317: Python doesn't need parens here > > - line 368: can this be re-flowed now? > > t_change_facet.py: > > - line 252: this doesn't read well. Perhaps you mean something like > "Verify that resetting a facet explicitly set to false restores the > delivered content."? > all addressed. > - I'll also point out that this test seems to do more than what either > the name or the docstring claim. Are we missing these basic tests > (like the graf starting at line 291) elsewhere? The last three tests > (from line 306 on) seem relevant. Not sure about the rest. > in the new test case there are 3 setup steps (for which i verify the results after each setup step), then one test that doesn't really match the docstring, and then 3 tests which match the docstring (and require the configuration created by the first three setup steps). i've moved the one test in the middle (which verifies that installing a random package doesn't change facets and doesn't match the docstring claim) into it's own test. i could create additional seperate tests for for the remaining 3 setup steps, and then re-write the test such that it does setup without any verification, but given how short the test case is, that seems kinda like overkill. let me know if this is ok. > - line 291: "an random" -> "a random" > done. _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
