On 07/27/12 10:18, Tim Foster wrote:
Hi Brock,
On 07/24/12 05:44 PM, Brock Pytlik wrote:
Webrev:
https://cr.opensolaris.org/action/browse/pkg/bpytlik/7185843-v1
Just a few nits really:
src/tests/api/t_bootenv.py:
line 52 doesn't need the '+=', '=' would be enough I think
Sure. I prefer the += because that means that if these branches get
reordered, or another inserted above it, this code doesn't need to
change, but unless I hear that that seems reasonable to other people,
I'll change it to '='.
line 62, I was thinking the wording of the first sentence could be
more explict, perhaps: "All other test suite tests test the BootEnv
class with, PKG_NO_LIVE_ROOT set in the environment, see the run()
method in Pkg5TestSuite and env_santize(..)"
Sure.
as I got a bit wary about the 'del' on line 73, without saving the
existing value.
line 87, do we need this given we set it in run(..) ->
env_sanitize(..) anyway?
Let's call it being extra safe. All the other places in the test suite
where we muck with the environment that I saw, we're always careful to
restore the environment to its previous state. That seems like good
practice to me.
Otherwise LGTM - looking forward to speedy test runs again!
cheers,
tim
Thanks for taking a look!
Brock
_______________________________________________
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
