Hi Sue -
Thanks for the review. See below.
ginnie
On 08/16/10 04:25 PM, Sue Sohn wrote:
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.
Thanks for catching this.
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.
ok.
83,86 indentation looks off
I think so too.
85, 91, 97 Are you depending on retval being set to B_FALSE here?
Seems like it would have been changed by line 75.
I'll double check it.
100-107 if transfer_log returns either B_TRUE or B_FALSE, you don't
need to set
retval on 105.
ok.
test_transfer_destonly.c
54,55,61,67 Similar comments about returning B_FALSE instead of retval
59,62 indentation is off
thanks.
test_set_logger_class.c
55-56 These lines are commented out. Remove?
57-66 overly generous with blank lines
Thanks. I'll make sure it's cleaned up.
tlogger.c
1-18 Should these comments be before the cddl/copyright?
I can move it.
47, 161 Commented out, remove line?
yes.
136 Need spaces after/before comment open/close
ok
212,218, 225, 231 need space after if
ok.
212-235 indentation
yes.
test_logger.py
43, 72, 73 Need a space after the #
Yes.
72-73 Something is off with this comment. back -> message ??
yes.
128 - is this a debug msg that should now be removed?
Yes. I should remove it.
245
Failed to find the critical in log
->
Failed to find the word, critical, in log ??
ok.
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?
I can add that check in.
test_report_progress.c
47 indentation
ok.
73 Should there be a return here?
It will just drop through to the cleanup, so I don't think it's necessary.
logger.py
168,196 Need space after #
thanks.
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
