On 1/4/10 8:12 AM, Keith Mitchell wrote: > Hi Evan (and the rest of the Error Service project team), > > I took a good look at errsvc.py, and a briefer look at the C files > (except for the test_*.c files). My comments are below. Looks pretty good! > > - Keith >
Hi Keith, Responding to the unittesting.py (and prototype_com) comments since Dermot has covered most of the rest... > unittesting.py: > > > General: The error cases should be tested too (e.g., calling > set_error_data with an invalid type, or with a value that mismatches the > type, or instantiating an ErrorInfo with an invalid type). This type of negative testing is now covered in the overall tests that are part of the test suite that Jason is delivering. Also much of that negative testing is also done as part of the test code for the C library. > > 31-32: Nit: platform.processor() is simple enough to make a call to, to > determine sparc or i386. Good point I'll change this. > 41: Nit: Instead of sys.exit(1), re-raise the exception (or raise a new > one). Calling sys.exit(1) prevents, for example, writing a meta-module > that runs a series of unittests (if the import of this module failed, > and a reasonable exception is raised, the managing module/script can > catch it and move on to the next set of tests - if sys.exit(1) is > called, the rest of the script wouldn't get run). Done > > 49: Nit: Call it "self.error_types", since it's a list with more than > one type. Done > > 52: Why store this value? It's easily retrievable with a call to len() This way we only have to retrieve the value once. While it's not necessary to store this value it's used here as a convenience. > > 57, 67, 72: Use "for error_type in self.error_types" instead of creating > the arbitrary index variable c. It's rare to need to know the index of > an item in a list in Python (and when you do, use of the enumerate() > function is the best way to track it when looping - line 116 is a good > example: "for idx, item in enumerate(self.error_types):") Done > > 66, 96, 115: This should be in a tearDown method. These are used here in the test to make sure we have a clean list of errors before starting the test and as such are in the proper place. > > 140: Should this be a liberrsvc constant? > Yes it should be. I've changed this to liberrsvc.ES_DATA_OP_STR but this is really something that should at some point be passed in... > prototype_com: > > General: The osol_install directory seems to be getting a little > cluttered, perhaps we need a 'utils' directory for things like the error > service, the future logging service, and so on? Possibly I can take a look at moving at least the errsvc files into a new subdir. However it's kind of a lower priority item and we may want to leave these in osol_install for now until we decide what other than liberrsvc should go into a separate utils subdir... Thanks for all your comments! -evan > > Evan Layton wrote: >> The CUD Error Service project team is requesting a code review of >> the project gate. >> >> The code implementation is complete and unit testing has been done. >> The official testing is in progress but is not yet complete. We will >> be providing updated incremental webrevs as code review comments and >> bug fixes come in. >> >> The webrev is available at: >> http://cr.opensolaris.org/~evanl/CUD_errsvc >> >> With the holiday coming up it appears that most people will be out >> of town for a while so we'll be giving extra time for folks to look >> at this. We'd like to have all comments back by January 15, 2010. >> >> Thanks! >> >> -evan >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
