diff --git a/docs/changes.rst b/docs/changes.rst index d769c62f4..e2c7aec65 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -41,6 +41,8 @@ Released: not yet outputs the compile error message and exception to stderr whereas before the compile error text was routed to stdout. (See issue #637) +* Fixed an issue where displaying instances in a table format missed properties + if the list of instances had different sets of properties. (See issue #650) **Enhancements:** @@ -62,6 +64,10 @@ Released: not yet the output of instances in table format now includes the value of the Values qualifier in parenthesis, in addition to the integer value. (See issue #634) +* The order of properties when displaying instances in a table format is now + predictable: First the sorted key properties, then the sorted non-key + properties. (Part of fix for issue #650) + **Cleanup** * Adds command to test connection for existence of the pull operations diff --git a/pywbemtools/pywbemcli/_common.py b/pywbemtools/pywbemcli/_common.py index d81470f22..ba065a596 100644 --- a/pywbemtools/pywbemcli/_common.py +++ b/pywbemtools/pywbemcli/_common.py @@ -29,6 +29,7 @@ except ImportError: from ordereddict import OrderedDict # pylint: disable=import-error +from pydicti import odicti import six import click import tabulate @@ -1404,12 +1405,19 @@ def _print_qual_decls_as_table(qual_decls, table_width, table_format): def _format_instances_as_rows(insts, max_cell_width=DEFAULT_MAX_CELL_WIDTH, - include_classes=False, context=None): + include_classes=False, context=None, + prop_names=None): """ Format the list of instances properties into as a list of the property values for each instance( a row of the table) gathered into a list of the rows. + The prop_names parameter is the list of (originally cased) property names + to be output, in the desired output order. It could be determined from + the instances, but since it is determined already by the caller, it + is passed in as an optimization. For test convenience, None is permitted + and causes the properties to again be determined from the instances. + Include_classes for each instance if True. Sets the classname as the first column. @@ -1432,18 +1440,9 @@ def _format_instances_as_rows(insts, max_cell_width=DEFAULT_MAX_CELL_WIDTH, if max_cell_width is None: max_cell_width = DEFAULT_MAX_CELL_WIDTH lines = [] - prop_names = [] - # find instance with max number of properties - - # TODO: The following code misses properties if there are instances with - # different sets of properties where no instance has all of them. - # See issue #650. - - for inst in insts: - pn = inst.keys() - if len(pn) > len(prop_names): - prop_names = pn + if prop_names is None: + prop_names = sorted_prop_names(insts) # Cache of ValueMapping objects for integer-typed properties. # Key: classname.propertyname, both in lower case. @@ -1523,13 +1522,7 @@ def _print_instances_as_table(insts, table_width, table_format, if table_width is None: table_width = DEFAULT_TABLE_WIDTH - # Find instance with max number of prop names to determine number - # of columns - prop_names = [] - for inst in insts: - pn = inst.keys() - if len(pn) > len(prop_names): - prop_names = pn + prop_names = sorted_prop_names(insts) # Try to estimate max cell width from number of cols # This allows folding long data. However it is incomplete in @@ -1561,13 +1554,49 @@ def _print_instances_as_table(insts, table_width, table_format, rows = _format_instances_as_rows(insts, max_cell_width=max_cell_width, include_classes=include_classes, - context=context) + context=context, prop_names=prop_names) title = 'Instances: {}'.format(insts[0].classname) click.echo(format_table(rows, new_header_line, title=title, table_format=table_format)) +def sorted_prop_names(insts): + """ + Return the list of (originally cased) property names that is the superset + of all properties in the input instances. + + The returned list has the key properties first, followed by the non-key + properties. Each group is sorted case insensitively. + + The key properties are determined from the instance paths, if present. + The function tolerates it if only some of the instances have a path, + and if instances of subclasses have additional keys. + """ + + all_props = odicti() # key: org prop name, value: lower cased prop name + key_props = odicti() # key: org prop name, value: lower cased prop name + for inst in insts: + inst_props = inst.keys() + for pn in inst_props: + all_props[pn] = pn.lower() + if inst.path: + key_prop_names = inst.path.keys() + for pn in inst_props: + if pn in key_prop_names: + key_props[pn] = pn.lower() + + nonkey_props = odicti() # key: org prop name, value: lower cased prop name + for pn in all_props: + if pn not in key_props: + nonkey_props[pn] = all_props[pn] + + key_prop_list = sorted(key_props.keys(), key=lambda p: p.lower()) + nonkey_prop_list = sorted(nonkey_props.keys(), key=lambda p: p.lower()) + key_prop_list.extend(nonkey_prop_list) + return key_prop_list + + def _print_objects_as_table(objects, output_format, context=None): """ Call the method for each type of object to print that object type diff --git a/tests/unit/simple_assoc_mock_model.mof b/tests/unit/simple_assoc_mock_model.mof index 0b55a555c..5cbccb6a5 100644 --- a/tests/unit/simple_assoc_mock_model.mof +++ b/tests/unit/simple_assoc_mock_model.mof @@ -70,41 +70,37 @@ class TST_FamilyCollection { instance of TST_Person as $Mike { name = "Mike"; - gender = 2; likes = {1, 2}; + gender = 2; }; instance of TST_Person as $Saara { name = "Saara"; - gender = 1; likes = {1}; + gender = 1; }; instance of TST_Person as $Sofi { name = "Sofi"; gender = 1; - likes = NULL; }; instance of TST_Person as $Gabi{ name = "Gabi"; + likes = {2}; gender = 1; - likes = {}; }; // Define instances of the TST_PersonSub instance of TST_PersonSub as $Mikesub{ name = "Mikesub"; gender = 2; - likes = {1, 2}; secondProperty = "one" ; counter = 1; }; instance of TST_PersonSub as $Saarasub { name = "Saarasub"; gender = 1; - likes = {2}; secondProperty = "two" ; counter = 2; }; instance of TST_PersonSub as $Sofisub{ name = "Sofisub"; gender = 1; - likes = NULL; secondProperty = "three" ; counter = 3; }; diff --git a/tests/unit/test_common.py b/tests/unit/test_common.py index 7d4e89fc5..b0a9f5706 100755 --- a/tests/unit/test_common.py +++ b/tests/unit/test_common.py @@ -2431,39 +2431,72 @@ def test_fold_strings(testcase, input_str, max_width, brk_long_wds, brk_hyphen, # TODO this is a pytest. param def simple_instance(pvalue=None): """ - Build a simple instance to test and return that instance. If the param - pvalue is provided it is a scalar value and the instance with just - this property is returned. + Build a simple instance to test and return that instance. The properties + in the instance are sorted by (lower cased) property name. + + If the parameter pvalue is provided, it must be a scalar value and an + instance with a single property with that value is returned. + """ + if pvalue: + properties = [CIMProperty("P", pvalue)] + else: + properties = [ + CIMProperty("Pbf", value=False), + CIMProperty("Pbt", value=True), + CIMProperty("Pdt", DATETIME1_OBJ), + CIMProperty("Pint32", Uint32(99)), + CIMProperty("Pint64", Uint64(9999)), + CIMProperty("Pstr1", u"Test String"), + ] + inst = CIMInstance("CIM_Foo", properties) + return inst + + +def simple_instance_unsorted(pvalue=None): + """ + Build a simple instance to test and return that instance. The properties + in the instance are not sorted by (lower cased) property name, but + the property order when sorted is the same as in the instance returned by + simple_instance(). + + If the parameter pvalue is provided, it must be a scalar value and an + instance with a single property with that value is returned. """ if pvalue: properties = [CIMProperty("P", pvalue)] else: - properties = [CIMProperty("Pbt", value=True), - CIMProperty("Pbf", value=False), - CIMProperty("Pint32", Uint32(99)), - CIMProperty("Pint64", Uint64(9999)), - CIMProperty("Pdt", DATETIME1_OBJ), - CIMProperty("Pstr1", u"Test String")] + properties = [ + CIMProperty("Pbt", value=True), + CIMProperty("Pbf", value=False), # out of order + CIMProperty("pdt", DATETIME1_OBJ), # lower cased + CIMProperty("PInt64", Uint64(9999)), # out of order when case ins. + CIMProperty("Pint32", Uint32(99)), + CIMProperty("Pstr1", u"Test String"), + ] inst = CIMInstance("CIM_Foo", properties) return inst def simple_instance2(pvalue=None): """ - Build a simple instance to test and return that instance. If the param - pvalue is provided it is a scalar value and the instance with just - this property is returned. + Build a simple instance to test and return that instance. The properties + in the instance are sorted by (lower cased) property name. + + If the parameter pvalue is provided, it must be a scalar value and an + instance with a single property with that value is returned. """ if pvalue: properties = [CIMProperty("P", pvalue)] else: - properties = [CIMProperty("Pbt", value=True), - CIMProperty("Pbf", value=False), - CIMProperty("Puint32", Uint32(4294967295)), - CIMProperty("Psint32", Sint32(-2147483648)), - CIMProperty("Pint64", Uint64(9999)), - CIMProperty("Pdt", DATETIME1_OBJ), - CIMProperty("Pstr1", u"Test String")] + properties = [ + CIMProperty("Pbf", value=False), + CIMProperty("Pbt", value=True), + CIMProperty("Pdt", DATETIME1_OBJ), + CIMProperty("Pint64", Uint64(9999)), + CIMProperty("Psint32", Sint32(-2147483648)), + CIMProperty("Pstr1", u"Test String"), + CIMProperty("Puint32", Uint32(4294967295)), + ] inst = CIMInstance("CIM_Foo", properties) return inst @@ -2477,27 +2510,27 @@ def string_instance(tst_str): return inst -# Testcases for format_inst_to_table() +# Testcases for _format_instances_as_rows() # Each list item is a testcase tuple with these items: # * desc: Short testcase description. # * kwargs: Keyword arguments for the test function: - # * args: CIMInstance(s) object to be tested and col_width field - # * kwargs: Dict of input args for tocimxml(). - # * exp_xml_str: Expected CIM-XML string, as a tuple/list of parts. + # * args: Positional args for _format_instances_as_rows(). + # * kwargs: Keyword args for _format_instances_as_rows(). + # * exp_rtn: Expected return value of _format_instances_as_rows(). # * exp_exc_types: Expected exception type(s), or None. # * exp_rtn: Expected warning type(s), or None. # * condition: Boolean condition for testcase to run, or 'pdb' for debugger -TESTCASES_FMT_INSTANCE_AS_ROWS = [ +TESTCASES_FORMAT_INSTANCES_AS_ROWS = [ ( "Verify simple instance to table", dict( args=([simple_instance()], None), kwargs=dict(), exp_rtn=[ - ["true", "false", "99", "9999", DATETIME1_STR, - u'"Test String"', ]], + ["false", "true", DATETIME1_STR, "99", "9999", + u'"Test String"']], ), None, None, True, ), @@ -2507,11 +2540,121 @@ def string_instance(tst_str): args=([simple_instance()], 30), kwargs=dict(), exp_rtn=[ - ["true", "false", "99", "9999", DATETIME1_STR, + ["false", "true", DATETIME1_STR, "99", "9999", u'"Test String"']], ), None, None, True, ), + ( + "Verify simple instance to table, unsorted", + dict( + args=([simple_instance_unsorted()], None), + kwargs=dict(), + exp_rtn=[ + ["false", "true", DATETIME1_STR, "99", "9999", + u'"Test String"']], + ), + None, None, True, ), + + ( + "Verify instance with 2 keys and 2 non-keys, unsorted", + dict( + args=(), + kwargs=dict( + insts=[ + CIMInstance( + "CIM_Foo", + properties=[ + CIMProperty("P2", value="V2"), + CIMProperty("p1", value="V1"), + CIMProperty("Q2", value="K2"), + CIMProperty("q1", value="K1"), + ], + path=CIMInstanceName( + "CIM_Foo", + keybindings=[ + CIMProperty("Q2", value="K2"), + CIMProperty("q1", value="K1"), + ] + ), + ), + ], + ), + exp_rtn=[ + ['"K1"', '"K2"', '"V1"', '"V2"'], + ], + ), + None, None, True, ), + + ( + "Verify 2 instances with different sets of properties", + dict( + args=(), + kwargs=dict( + insts=[ + CIMInstance( + "CIM_Foo", + properties=[ + CIMProperty("P2", value="VP2a"), + CIMProperty("p1", value="VP1a"), + CIMProperty("P3", value="VP3a"), + ], + ), + CIMInstance( + "CIM_FooSub", + properties=[ + CIMProperty("P2", value="VP2b"), + CIMProperty("p1", value="VP1b"), + CIMProperty("N1", value="VN1b"), + ], + ), + ], + ), + exp_rtn=[ + ['', '"VP1a"', '"VP2a"', '"VP3a"'], + ['"VN1b"', '"VP1b"', '"VP2b"', ''], + ], + ), + None, None, True, ), + + ( + "Verify 2 instances where second one has path", + dict( + args=(), + kwargs=dict( + insts=[ + CIMInstance( + "CIM_Foo", + properties=[ + CIMProperty("P2", value="VP2a"), + CIMProperty("p1", value="VP1a"), + ], + ), + CIMInstance( + "CIM_FooSub", + properties=[ + CIMProperty("P2", value="VP2b"), + CIMProperty("p1", value="VP1b"), + CIMProperty("Q2", value="K2b"), + CIMProperty("q1", value="K1b"), + ], + path=CIMInstanceName( + "CIM_Foo", + keybindings=[ + CIMProperty("q2", value="K2b"), + CIMProperty("Q1", value="K1b"), + ] + ), + ), + ], + ), + exp_rtn=[ + ['', '', '"VP1a"', '"VP2a"'], + ['"K1b"', '"K2b"', '"VP1b"', '"VP2b"'], + ], + ), + None, None, True, ), + ( "Verify simple instance with one string all components overflow line", dict( @@ -2641,11 +2784,11 @@ def string_instance(tst_str): @pytest.mark.parametrize( "desc, kwargs, exp_exc_types, exp_warn_types, condition", - TESTCASES_FMT_INSTANCE_AS_ROWS) + TESTCASES_FORMAT_INSTANCES_AS_ROWS) @simplified_test_function -def test_format_insts_as_rows(testcase, args, kwargs, exp_rtn): +def test_format_instances_as_rows(testcase, args, kwargs, exp_rtn): """ - Test the output of the common:format_insts_as_table function + Test the output of the common _format_instances_as_rows() function """ # The code to be tested @@ -2672,19 +2815,19 @@ def test_format_insts_as_rows(testcase, args, kwargs, exp_rtn): format(testcase.desc, exp_rtn, act_rtn) -# Testcases for format_inst_to_table() +# Testcases for _print_instances_as_table() # Each list item is a testcase tuple with these items: # * desc: Short testcase description. # * kwargs: Keyword arguments for the test function: - # * args: CIMInstance(s) object to be tested, col_width field, ouput_fmt - # * kwargs: Dict of input args for tocimxml(). + # * args: Positional args for _print_instances_as_table(). + # * kwargs: Keyword args for _print_instances_as_table(). # * exp_stdout: Expected output on stdout. # * exp_exc_types: Expected exception type(s), or None. # * exp_warn_types: Expected warning type(s), or None. # * condition: Boolean condition for testcase to run, or 'pdb' for debugger -TESTCASES_PRINT_INSTANCE_AS_TABLE = [ +TESTCASES_PRINT_INSTANCES_AS_TABLE = [ ( "Verify print of simple instance to table", dict( @@ -2692,10 +2835,10 @@ def test_format_insts_as_rows(testcase, args, kwargs, exp_rtn): kwargs=dict(), exp_stdout="""\ Instances: CIM_Foo -Pbt Pbf Pint32 Pint64 Pdt Pstr1 ------ ----- -------- -------- ----------------------- ------------- -true false 99 9999 "20140922104920.524789" "Test String" - "+000" +Pbf Pbt Pdt Pint32 Pint64 Pstr1 +----- ----- ----------------------- -------- -------- ------------- +false true "20140922104920.524789" 99 9999 "Test String" + "+000" """, ), None, None, not CLICK_ISSUE_1590 @@ -2707,12 +2850,12 @@ def test_format_insts_as_rows(testcase, args, kwargs, exp_rtn): kwargs=dict(), exp_stdout="""\ Instances: CIM_Foo -Pbt Pbf Puint32 Psint32 Pint64 Pdt Pstr1 ------ ----- ---------- ----------- -------- --------- -------- -true false 4294967295 -2147483648 9999 "2014092" "Test " - "2104920" "String" - ".524789" - "+000" +Pbf Pbt Pdt Pint64 Psint32 Pstr1 Puint32 +----- ----- --------- -------- ----------- -------- ---------- +false true "2014092" 9999 -2147483648 "Test " 4294967295 + "2104920" "String" + ".524789" + "+000" """, ), None, None, not CLICK_ISSUE_1590 @@ -2745,8 +2888,8 @@ def test_format_insts_as_rows(testcase, args, kwargs, exp_rtn): @pytest.mark.parametrize( "desc, kwargs, exp_exc_types, exp_warn_types, condition", - TESTCASES_PRINT_INSTANCE_AS_TABLE) -def test_print_insts_as_table( + TESTCASES_PRINT_INSTANCES_AS_TABLE) +def test_print_instances_as_table( desc, kwargs, exp_exc_types, exp_warn_types, condition, capsys): """ Test the output of the print_insts_as_table function. This primarily diff --git a/tests/unit/test_instance_cmds.py b/tests/unit/test_instance_cmds.py index 1b8a48607..782c02ed4 100644 --- a/tests/unit/test_instance_cmds.py +++ b/tests/unit/test_instance_cmds.py @@ -380,14 +380,13 @@ instance of TST_Person { name = "Gabi"; + likes = { 2 }; gender = 1; - likes = { }; }; instance of TST_Person { name = "Sofi"; gender = 1; - likes = NULL; }; """ @@ -958,9 +957,9 @@ {'stdout': """ Instances: PyWBEM_AllTypes +-----------------+------------+--------------+--------------+ -| InstanceId | scalBool | scalUint32 | scalSint32 | +| InstanceId | scalBool | scalSint32 | scalUint32 | +=================+============+==============+==============+ -| "test_instance" | true | 9999 | -9999 | +| "test_instance" | true | -9999 | 9999 | +-----------------+------------+--------------+--------------+ """, 'rc': 0, @@ -976,9 +975,9 @@ {'stdout': """ Instances: PyWBEM_AllTypes +-----------------+-------------+---------------+---------------+ -| InstanceId | arrayBool | arrayUint32 | arraySint32 | +| InstanceId | arrayBool | arraySint32 | arrayUint32 | +=================+=============+===============+===============+ -| "test_instance" | true, false | 0, 9999 | 0, -9999 | +| "test_instance" | true, false | 0, -9999 | 0, 9999 | +-----------------+-------------+---------------+---------------+ """, @@ -986,18 +985,22 @@ 'test': 'linesnows'}, ALLTYPES_MOCK_FILE, OK], - ['Verify instance command enumerate of TST_PersonSub shows value-mapped ' + ['Verify instance command enumerate of TST_Person shows value-mapped ' 'properties in table output', - {'args': ['enumerate', 'TST_PersonSub', '--pl', 'name,gender,likes'], + {'args': ['enumerate', 'TST_Person', '--pl', 'name,gender,likes'], 'general': ['--output-format', 'table']}, {'stdout': """ -Instances: TST_PersonSub +Instances: TST_Person +------------+------------+-----------------------+ | name | gender | likes | |------------+------------+-----------------------| +| "Gabi" | 1 (female) | 2 (movies) | +| "Mike" | 2 (male) | 1 (books), 2 (movies) | +| "Saara" | 1 (female) | 1 (books) | +| "Sofi" | 1 (female) | | | "Gabisub" | 1 (female) | | -| "Mikesub" | 2 (male) | 1 (books), 2 (movies) | -| "Saarasub" | 1 (female) | 2 (movies) | +| "Mikesub" | 2 (male) | | +| "Saarasub" | 1 (female) | | | "Sofisub" | 1 (female) | | +------------+------------+-----------------------+ @@ -2566,7 +2569,7 @@ 'test': 'linesnows'}, ASSOC_MOCK_FILE, OK], - ['Verify instance command count *Person* with --association', + ['Verify instance command count *TST_* with --association', {'args': ['count', '*TST_*', '--association'], 'general': ['--default-namespace', 'interop', '--output-format', 'plain']}, @@ -2579,7 +2582,7 @@ 'test': 'linesnows'}, QUALIFIER_FILTER_MODEL, OK], - ['Verify instance command count *Person* with --experimental', + ['Verify instance command count *TST_* with --experimental', {'args': ['count', '*TST_*', '--experimental'], 'general': ['--default-namespace', 'interop', '--output-format', 'plain']}, @@ -2591,7 +2594,7 @@ 'test': 'linesnows'}, QUALIFIER_FILTER_MODEL, OK], - ['Verify instance command count *Person* with --indication', + ['Verify instance command count *TST_* with --indication', {'args': ['count', '*TST_*', '--no-indication', '--association'], 'general': ['--default-namespace', 'interop', '--output-format', 'plain']},