Hi Karen.
Here are my comments on the test files, submitted late with your permission.
usr/src/lib/install_engine/test/__init__.py: OK
-------------------------------------------
usr/src/lib/install_engine/test/empty_checkpoint.py:
---------------------------------------------------
''' missing from method and class declarations
usr/src/lib/install_engine/test/engine_test_utils.py
----------------------------------------------------
59: reset_engine needs '''
60: What if old_engine is not None? Don't we still want to clear its
DOC? As is, the old_engine is ignored if not None.
usr/src/lib/install_engine/test/test_common.py
----------------------------------------------
40: class declaration is missing '''
test_get_argspec_function as is seems to do the same thing twice as is.
Would it be better to check the argspec returned by get_argspec for each
of basic_function and lambda_func, since they are very different from
each other?
usr/src/lib/install_engine/test/test_cp_data.py
-----------------------------------------------
Class and method declarations are missing '''
Nit: Due to common code, test_relative_load() and test_absolute_load()
can easily be implemented in terms of a single method, taking "." or
abs_path as an argument for the modpath.
usr/src/lib/install_engine/test/test_engine.py
----------------------------------------------
Several class and method declarations are missing """ docstring comments.
Several docstring comments in the latter half of this file have an extra
\" before the final """.
158: Get rid of try/except around shutil.rmtree and specify
ignore_errors=True as an argument instead.
235, 248: Both of these methods are called
test_check_callback_bound_class_method(). It is not clear what bound or
unbound pertains to. A method is bound to an instance. A function is
bound to a class (meaning no instance is needed and there is only one
for the class).
From looking at _check_callback line 698-699, it is clear that the
binding is to an instance. Based on this, I suggest:
- change the method name on 235 to be
test_check_callback_instance_bound_method()
- change the method name on 248 to be
test_check_callback_non_instance_bound_function()
- Fix the comments on 249-250
- remove "self" on line 252
448: indentation
484: not necessary?
497: not necessary?
499: method is incomplete. Assertion (and more) is missing.
551: I think a snapshot needs to be added to
test_out_process_rollback_no_dataset(), else
self.engine._rollback() can raise an exception due to the missing
snapshot, as much as it can for a missing dataset. (The following test
at 560 tests the case where there is a dataset but no snapshot.)
599: No lambda function is needed. Just say:
self.engine.get_cp_data = MockCheckpointData
(no () ).
811: The variable "dir_name" can be confusing here because
os.path.dirname() is the counterpart to os.path.basename(). Please add
a comment to clarify that
you got a path for a directory, and so that final branch of the path is
a directory and not a file.
813: Does the directory just created get cleaned up?
815: The name here is the same as on 801. I suggest
test_gen_tmp_dir_w_env()
822: Suggest moving assertion to below 826 so cleanup on 825-826 will
always happen.
887: Suggest comment clarification:"Verify that registering checkpoint
works..."
917: If the point of this test is to check processing when a checkpoint
name is forgotten, then this test is correct. If it is to check
processing when a blank or missing (None) checkpoint is passed, then an
additional arg of None or "" is needed on line 917.
947: similar comment as for 917.
970: similar comment as for 917.
1039-1043: If it's not OK to not get passed a keyword arg that is
anticipated, this should be self.assertRaises() here since a failure is
expected.
1075-1079: likewise
1088-1092: likewise
The tests distinguish
- when 1 KW arg is expected and none are provided (fail),
test_reg_expect_1_kw_arg_provide_none()
- when 1 KW arg is expected and one is provided (fail),
test_reg_expect_1_kw_arg_provide_1()
- when 1 KW arg is expected and two are provided (fail),
test_reg_expect_1_kw_arg_provide_2()
- when multiple KW arg are expected and none are provided (fail),
test_reg_expect_multi_kw_arg_provide_none()
- when multiple KW arg are expected and one is provided (fail),
test_reg_expect_multi_kw_arg_provide_1()
- when multiple KW arg are expected and multiple are provided (fail),
test_reg_expect_multi_kw_arg_provide_multiple()
However, in checkpoint_data.py / validate_function_args(), all I see for
checking keyword args is that some amount are passed when none are
expected. (This is lines 91 - 97.) Thus it looks like either the tests
are too rigorous or the code it is testing needs more development.
1110: The header comment says multiple keyword args, but only one is
passed on line 1116.
1134: I don't understand the header comment. What is the goal of this
method?
1138: chkpt.cp_info.checkpoint_class_name arg is missing.
1194-1195, 1198-1199: use list extend().
1329, 1383: Hardwired value. Where does 15 come from?
I don't understand how on 1307, the cancel checkpoint is registered
before the third checkpoint, but after the cancel is issued, the
cancelled checkpoint is one (on line 1335). Please explain.
1347: Assertion is missing from this test.
1363: The assertion that checks what the header says to check, is missing.
There are many places where self.check_result() is called after
self.assertRaises() is called. An assertRaises call implies that an
exception is expected, so I would assume after that point that the state
is invalid and there is nothing to check. If I am correct, such calls
to check_result() should be removed.
usr/src/lib/install_engine/test/test_engine_complex.py
------------------------------------------------------
Several class and method declarations are missing """ docstring comments.
92: Where does self._testMethodName come from? I couldn't find it.
94: hardwired value: where does 15 come from?
116: Method name is too generic.
136: I don't see how this checks that a snapshot was overwritten
(comparing with old data, for example), only that a snapshot is there.
Does the old snapshot not have cp_data.data_cache_path maybe?
160+ Please add a blank line here.
213: Comment doesn't really explain the goal here, that it goes all the
way through after execing only the first two checkpoints.
231: Get rid of the list, which can be seen as duplicated code:
self.verify_resumable_cp_result(self.name_list[:3], cp_list)
239: Use the original list as much as possible:
self.verify_resumable_cp_result(self.name_list[0:1], cp_list)
301: [:] not needed
352: s/subsequent/previous/
357-358: reverse these two lines.
385: Is this needed?
461: Why not check that all snapshots are not there?
473: There could still be a window where checkpoint n+1 has finished and
checkpoint n hasn't started yet. If the spinning checkpoint can be made
the first one, there will be no such window.
Thanks,
Jack
On 09/22/10 16:00, Karen Tung wrote:
Hi,
The install engine implementation is ready for code review. The webrev
is at:
http://cr.opensolaris.org/~ktung/cud-engine/
All the Makefile related changes made in the usr/src/ and usr/src/lib are
temporary changes needed for the gate to build cleanly. They might
be the same changes the logging and Data Object Cache projects made.
When those 2 projects integrate into the slim_source gate, I will
update the
Makefiles as needed before integration.
All tests implemented in the test directory of the engine code are
executed
successfully. All the engine API are also being used by the
Distribution Construction
project as they move DC over to using CUD.
I would like to get at least 2 reviewers. If you plan to review all
or part of the
code, please email me privately so I can make sure the code get good
review coverage.
Please send all your comments by COB 10/6/2010.
Thanks,
--Karen
_______________________________________________
caiman-discuss mailing list
caiman-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
caiman-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
