Hi Dermot, Dermot McCluskey wrote: > Keith, > > Just addressing one point I skipped previously: > > > > unittesting.py: > > > ... > > 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). > > > unittesting.py is an executable script and is not intended > to be imported elsewhere. It's a quick way for developers to > check 'did my recent changes break anything?'. There's a > separate set of TET test cases being written elsewhere. > > So, given all that, I think it's much neater for the script > to exit gracefully with an error message rather than throw > out an ugly traceback, in this instance. > > Do you agree?
I still disagree. It would be good to have a unified, automated test framework for running unit tests down the line. The portion of said framework for testing Python would need to be able to continue running even if a single module's unit tests failed. And since it is meant for developers, not end users, I don't see a problem with an ugly traceback (especially on an import failure, which would indicate a very strange situation in which the developer would likely want the error message from the ImportError). By the way, I think the unit test module with your code is great! It gives us a starting point for building such a test framework. - Keith > > > - Dermot > > > > On 01/04/10 15:12, 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 >> >> errsvc.py: >> >> General nit: There are some extra spaces around parens in this file >> (e.g. at lines 38, 57, 110, etc.) >> >> 40-42: Can we define this list as a constant somewhere? >> >> 49-55: Is there any reason to not use direct attribute access? >> >> 57: Why the use of return codes in Python code? >> >> 65: Why this syntax? If this list is expected to grow later, can we >> define it as a constant of the class? >> >> 88: Performance is likely not an issue here, but in general, string >> concatenation is much more efficient if you create a list of strings, >> and then use "\n".join(<list>) or something similar. >> >> 110-118: Could this be incorporated into the ErrorInfo.__init__? Or, >> is it a valid scenario to create an ErrorInfo independently from the >> list? >> >> 133: Since we control the adding of errors to the list, would it be >> easier to simply store separate lists for each type? Or have the >> global __errors be a dictionary, where the key is error_type and the >> value is a list of errors of that type? It seems that conglomerating >> lists together when the full list of all errors is needed would be >> simpler than picking apart the list to get errors of a certain type. >> >> 143: Same question. Since we control adding items to __errors via the >> create_error_info method, it seems like managing multiple lists (or >> dictionaries, as appropriate) would be relatively simple. >> >> 157: The second check (errs > 0) is redundant - "if errs:" only >> returns True if a list is non-empty. >> 158: Nit: "for err in errs:" >> 159: Nit: "print err" is sufficient. (And, using str(err) instead of >> err.__str__() is generally more common) >> >> 178: Nit: integer -> string >> >> 163-185: Can you use the Python built-in isinstance instead? >> >> 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). >> >> 31-32: Nit: platform.processor() is simple enough to make a call to, >> to determine sparc or i386. >> 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). >> >> 49: Nit: Call it "self.error_types", since it's a list with more than >> one type. >> >> 52: Why store this value? It's easily retrievable with a call to len() >> >> 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):") >> >> 66, 96, 115: This should be in a tearDown method. >> >> 140: Should this be a liberrsvc constant? >> >> errsvc.c: >> >> General question: What's the advantage/purpose of the use of >> threading in this code? >> >> General note: There are a few comments that are just "Call Python" - >> it would help if the comment were expanded to indicate what Python >> code is called (especially since the Python method called can get >> obscured by the fact that it takes quite a few lines of C code to get >> to the point of being able to call said method) >> >> 61: What's the purpose of hardcoding the path here? Seems like this >> would cause maintenance problems and reduce flexibility. >> >> 110: "UNKNOWN ERROR" - should this be #defined? >> >> 566-571: I don't quite understand the comment. It seems like calling >> the clear_error_list function should take care of clearing out the >> error list. Where are the extra references coming from that need to >> be decremented here, instead of by whichever portion of the code >> grabbed hold of the extra reference in the first place? >> >> 683: As mentioned above, Python code should raise errors, and not >> return error codes. Reserve return values for useful data, and check >> for exceptions if attempting to handle an error condition. >> >> 1030-1078: This bit of code looks very similar to blocks of code in >> other functions. Is it possible to combine somehow for the purpose of >> simplifying, or are they different enough that it's not possible to >> do such a thing in C? >> >> liberrsvc.h:60: Nit: _LIBERREVC_H -> _LIBERRSVC_H >> >> liberrsvc_defs.h: >> >> 38-40, 41-42: Nit: Make these into single block comments so they're >> more readable. >> >> 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? >> >> 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 >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
