On 08/09/10 16:11, Ginnie Wray wrote:
Hi -
I've incorporated the changes from the last code review.
I would like feedback for these changes by Monday Aug 16th.
Code is located at:
http://cr.opensolaris.org/~ginnie/logger0809/
Hi Ginnie,
Took a quick look and found a few minor things:
test_setup.c, test_transfer_destonly.c, and test_transfer_srclog.c.
test_set_logger_class.c, test_set_log_level.c, test_report_progress.c,
test/Makefile,test_add_progress_handler.c,
test_addhandler.c,test_addhandler_fail.c,test_close_logging.c,
test_get_logger.c,test_addhandler_fail.c
--> Need to update to Oracle copyright in above files.
test_transfer_srclog.c
54 - remove this line? Actually, rather than returning retval on 55, 61 and 67,
I'd just return B_FALSE, since retval isn't really being set.
83,86 indentation looks off
85, 91, 97 Are you depending on retval being set to B_FALSE here? Seems like it
would have been changed by line 75.
100-107 if transfer_log returns either B_TRUE or B_FALSE, you don't need to set
retval on 105.
test_transfer_destonly.c
54,55,61,67 Similar comments about returning B_FALSE instead of retval
59,62 indentation is off
test_set_logger_class.c
55-56 These lines are commented out. Remove?
57-66 overly generous with blank lines
tlogger.c
1-18 Should these comments be before the cddl/copyright?
47, 161 Commented out, remove line?
136 Need spaces after/before comment open/close
212,218, 225, 231 need space after if
212-235 indentation
test_logger.py
43, 72, 73 Need a space after the #
72-73 Something is off with this comment. back -> message ??
128 - is this a debug msg that should now be removed?
245
Failed to find the critical in log
->
Failed to find the word, critical, in log ??
269-297 These tests seem to ensure that the text is in the log file. Is there a
way to verify that they were logged at the right level?
test_report_progress.c
47 indentation
73 Should there be a return here?
logger.py
168,196 Need space after #
Sue
I've also included output from the nose tests:
bash-4.0$ ./slim_test lib/install_logging_pymod/test --with-coverage
#1 Ensure that FileHandlers can be added to a logger ... ok
#2 Ensure that StreamHandlers can be added to a logger ... ok
#3 Ensure that InstallLogger close works ... ok
#4 Ensure default_log is created and uses the default format. ... ok
#5 Ensure that only one InstallLogger is created for Install Logger ... ok
#6 Ensure that default format is set for the InstallLogger ... ok
#7 Ensure that a log message below the designated level does not log ... ok
#8 Ensure that default log is returned with default_log method ... ok
#9 Ensure that logger name is returned correctly ... ok
#10 Ensure that logger instance is from InstallLogger ... ok
#11 Ensure that debug log messages are logged to the log file ... ok
#12 Ensure that info log messages are logged to the log file ... ok
#13 Ensure that warning log messages are logged to the log file ... ok
#14 Ensure that default log transfers to destination ... ok
#15 Ensure that source file transfers to desired destination ... ok
#16 Test the message returned by ProgressLogRecord get_message ... ok
#17 Test that a second progress handler can be added ... ok
#18 'Tests that progress is reported to the default log ... ok
#19 Test that progress is reported to the progress receiver ... ok
#20 Tests that report_progress fails is value is invalid ... ok
Name Stmts Exec Cover Missing
---------------------------------------------------
osol_install 1 1 100%
osol_install.logger 151 126 83% 73-74, 83, 122, 134-135,
171-179, 187-190, 229, 264, 268, 282-287
---------------------------------------------------
TOTAL 152 127 83%
----------------------------------------------------------------------
Ran 20 tests in 2.300s
OK
Thanks,
ginnie
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
