Code optimization; review comments; updates based on converting tests

- Pulling common code of getting duts in testbed into a function.
  - There was code to create TestbedInfo by parsing the testbed-file and getting the testbed info for a testbed.
    This code was repeated in tbinfo fixture and all the parameterization helper methods.
    Moved this into another function 'get_tbinfo' and made other functions/fixtures call this function.

- Review comments:
   - When selecting a frontend node, if we find that none of the DUTs in the testbed
     are frontend nodes, then we should fail.

- scope of parameterized fixtures
   - By default, all the 'enum_dut*' fixtures in pytest_generate_tests were created with scope 'function'.
     This made these fixtures not usable in other fixtures that have 'module' scope.
   - Added scope to be 'module' to all the 'enum_dut*' fixtures in 'pytest_generate_tests'.

Author: sanmalho-git

tests/conftest.py

@@ -142,18 +142,25 @@ def config_logging(request):
         dataplane_logger.setLevel(logging.ERROR)
 
 
-@pytest.fixture(scope="session")
-def tbinfo(request):
+def get_tbinfo(request):
     """
-    Create and return testbed information
+    Helper function to create and return testbed information
     """
     tbname = request.config.getoption("--testbed")
     tbfile = request.config.getoption("--testbed_file")
     if tbname is None or tbfile is None:
         raise ValueError("testbed and testbed_file are required!")
 
     testbedinfo = TestbedInfo(tbfile)
-    return testbedinfo.testbed_topo.get(tbname, {})
+    return tbname, testbedinfo.testbed_topo.get(tbname, {})
+
+@pytest.fixture(scope="session")
+def tbinfo(request):
+    """
+    Create and return testbed information
+    """
+    _, testbedinfo = get_tbinfo(request)
+    return testbedinfo
 
 
 @pytest.fixture(name="duthosts", scope="session")
@@ -190,7 +197,9 @@ def duthost(duthosts, request):
 def rand_one_dut_hostname(request):
     """
     """
-    dut_hostnames = generate_params_dut_hostname(request)
+    tbname, testbedinfo = get_tbinfo(request)
+    duts_in_testbed = testbedinfo["duts"]
+    dut_hostnames = generate_params_dut_hostname(duts_in_testbed, tbname)
     if len(dut_hostnames) > 1:
         dut_hostnames = random.sample(dut_hostnames, 1)
     return dut_hostnames[0]
@@ -199,9 +208,10 @@ def rand_one_dut_hostname(request):
 def rand_one_frontend_dut_hostname(request):
     """
     """
-    frontend_dut_hostnames = generate_params_frontend_hostname(request)
-    if len(frontend_dut_hostnames) > 1:
-        dut_hostnames = random.sample(frontend_dut_hostnames, 1)
+    tbname, testbedinfo = get_tbinfo(request)
+    duts_in_testbed = testbedinfo["duts"]
+    frontend_dut_hostnames = generate_params_frontend_hostname(request, duts_in_testbed, tbname)
+    dut_hostnames = random.sample(frontend_dut_hostnames, 1)
     return dut_hostnames[0]
 
 
@@ -582,24 +592,20 @@ def get_host_data(request, dut):
 
     return inv_data
 
-def generate_params_frontend_hostname(request):
-    tbname = request.config.getoption("--testbed")
-    tbfile = request.config.getoption("--testbed_file")
-    if tbname is None or tbfile is None:
-        raise ValueError("testbed and testbed_file are required!")
-
-    tbinfo = TestbedInfo(tbfile)
+def generate_params_frontend_hostname(request, duts_in_testbed, tbname):
     frontend_duts = []
     inv_files = get_inventory_files(request)
-    for dut in tbinfo.testbed_topo[tbname]["duts"]:
+    for dut in duts_in_testbed:
         if is_frontend_node(inv_files, dut):
             frontend_duts.append(dut)
-
+    assert len(frontend_duts) > 0, \
+        "Test selected require at-least one frontend node, " \
+        "none of the DUTs '{}' in testbed '{}' are a supervisor node".format(duts_in_testbed, tbname)
     return frontend_duts
 
 
-def generate_params_frontend_hostname_rand_per_hwsku(request):
-    frontend_hosts = generate_params_frontend_hostname(request)
+def generate_params_frontend_hostname_rand_per_hwsku(request, duts_in_testbed, tbname):
+    frontend_hosts = generate_params_frontend_hostname(request, duts_in_testbed, tbname)
     inv_files = get_inventory_files(request)
     # Create a list of hosts per hwsku
     host_hwskus = {}
@@ -626,39 +632,26 @@ def generate_params_frontend_hostname_rand_per_hwsku(request):
     return frontend_hosts_per_hwsku
 
 
-def generate_params_supervisor_hostname(request):
-    tbname = request.config.getoption("--testbed")
-    tbfile = request.config.getoption("--testbed_file")
-    if tbname is None or tbfile is None:
-        raise ValueError("testbed and testbed_file are required!")
-
-    tbinfo = TestbedInfo(tbfile)
-
-    if len(tbinfo.testbed_topo[tbname]["duts"]) == 1:
+def generate_params_supervisor_hostname(request, duts_in_testbed, tbname):
+    if len(duts_in_testbed) == 1:
         # We have a single node - dealing with pizza box, return it
-        return [tbinfo.testbed_topo[tbname]["duts"][0]]
+        return [duts_in_testbed[0]]
     inv_files = get_inventory_files(request)
-    for dut in tbinfo.testbed_topo[tbname]["duts"]:
+    for dut in duts_in_testbed:
+        # Expecting only a single supervisor node
         if is_supervisor_node(inv_files, dut):
             return [dut]
-    pytest.fail("Test selected require a supervisor node, none of the DUTs '{}' are a supervisor node".format(tbinfo.testbed_topo[tbname]["duts"]))
+    pytest.fail("Test selected require a supervisor node, " +
+                "none of the DUTs '{}' in testbed '{}' are a supervisor node".format(duts_in_testbed, tbname))
 
 
-def generate_param_asic_index(request, dut_indices, param_type):
+def generate_param_asic_index(request, duts_in_testbed, dut_indices, param_type):
     logging.info("generating {} asic indicies for  DUT [{}] in ".format(param_type, dut_indices))
-
-    tbname = request.config.getoption("--testbed")
-    tbfile = request.config.getoption("--testbed_file")
-    if tbname is None or tbfile is None:
-        raise ValueError("testbed and testbed_file are required!")
-
-    tbinfo = TestbedInfo(tbfile)
-
     #if the params are not present treat the device as a single asic device
     asic_index_params = [DEFAULT_ASIC_ID]
 
     for dut_id in dut_indices:
-        dut = tbinfo.testbed_topo[tbname]["duts"][dut_id]
+        dut = duts_in_testbed[dut_id]
         inv_data = get_host_data(request, dut)
         if inv_data is not None:
             if param_type == ASIC_PARAM_TYPE_ALL and ASIC_PARAM_TYPE_ALL in inv_data:
@@ -669,26 +662,16 @@ def generate_param_asic_index(request, dut_indices, param_type):
                 dut_id, dut, asic_index_params))
     return asic_index_params
 
-def generate_params_dut_index(request):
-    tbname = request.config.getoption("--testbed")
-    tbfile = request.config.getoption("--testbed_file")
-    if tbname is None or tbfile is None:
-        raise ValueError("testbed and testbed_file are required!")
-    tbinfo = TestbedInfo(tbfile)
-    num_duts = len(tbinfo.testbed_topo[tbname]["duts"])
-    logging.info("Num of duts in testbed topology {}".format(num_duts))
+
+def generate_params_dut_index(duts_in_testbeds, tbname):
+    num_duts = len(duts_in_testbeds)
+    logging.info("Num of duts in testbed '{}' is {}".format(tbname, num_duts))
     return range(num_duts)
 
 
-def generate_params_dut_hostname(request):
-    tbname = request.config.getoption("--testbed")
-    tbfile = request.config.getoption("--testbed_file")
-    if tbname is None or tbfile is None:
-        raise ValueError("testbed and testbed_file are required!")
-    tbinfo = TestbedInfo(tbfile)
-    duts = tbinfo.testbed_topo[tbname]["duts"]
-    logging.info("DUTs in testbed topology: {}".format(str(duts)))
-    return duts
+def generate_params_dut_hostname(duts_in_testbed, tbname):
+    logging.info("DUTs in testbed '{}' are: {}".format(tbname, str(duts_in_testbed)))
+    return duts_in_testbed
 
 
 def generate_port_lists(request, port_scope):
@@ -797,29 +780,32 @@ def generate_priority_lists(request, prio_scope):
 def pytest_generate_tests(metafunc):
     # The topology always has atleast 1 dut
     dut_indices = [0]
-
+    tbname, testbedinfo = get_tbinfo(metafunc)
+    duts_in_testbed = testbedinfo["duts"]
     # Enumerators ("enum_dut_index", "enum_dut_hostname", "rand_one_dut_hostname") are mutually exclusive
     if "enum_dut_index" in metafunc.fixturenames:
-        dut_indices = generate_params_dut_index(metafunc)
-        metafunc.parametrize("enum_dut_index",dut_indices)
+        dut_indices = generate_params_dut_index(duts_in_testbed, tbname)
+        metafunc.parametrize("enum_dut_index", dut_indices, scope="module")
     elif "enum_dut_hostname" in metafunc.fixturenames:
-        dut_hostnames = generate_params_dut_hostname(metafunc)
-        metafunc.parametrize("enum_dut_hostname", dut_hostnames)
+        dut_hostnames = generate_params_dut_hostname(duts_in_testbed, tbname)
+        metafunc.parametrize("enum_dut_hostname", dut_hostnames, scope="module")
     elif "enum_supervisor_dut_hostname" in metafunc.fixturenames:
-        supervisor_hosts = generate_params_supervisor_hostname(metafunc)
-        metafunc.parametrize("enum_supervisor_dut_hostname", supervisor_hosts)
+        supervisor_hosts = generate_params_supervisor_hostname(metafunc, duts_in_testbed, tbname)
+        metafunc.parametrize("enum_supervisor_dut_hostname", supervisor_hosts, scope="module")
     elif "enum_frontend_dut_hostname" in metafunc.fixturenames:
-        frontend_hosts = generate_params_frontend_hostname(metafunc)
-        metafunc.parametrize("enum_frontend_dut_hostname", frontend_hosts)
+        frontend_hosts = generate_params_frontend_hostname(metafunc, duts_in_testbed, tbname)
+        metafunc.parametrize("enum_frontend_dut_hostname", frontend_hosts, scope="module")
     elif "enum_rand_one_per_hwsku_frontend_hostname" in metafunc.fixturenames:
-        frontend_hosts_per_hwsku = generate_params_frontend_hostname_rand_per_hwsku(metafunc)
-        metafunc.parametrize("enum_rand_one_per_hwsku_frontend_hostname", frontend_hosts_per_hwsku)
+        frontend_hosts_per_hwsku = generate_params_frontend_hostname_rand_per_hwsku(metafunc, duts_in_testbed, tbname)
+        metafunc.parametrize("enum_rand_one_per_hwsku_frontend_hostname", frontend_hosts_per_hwsku, scope="module")
 
 
     if "enum_asic_index" in metafunc.fixturenames:
-        metafunc.parametrize("enum_asic_index",generate_param_asic_index(metafunc, dut_indices, ASIC_PARAM_TYPE_ALL))
+        metafunc.parametrize("enum_asic_index",
+                             generate_param_asic_index(metafunc, duts_in_testbed, tbname, dut_indices, ASIC_PARAM_TYPE_ALL))
     if "enum_frontend_asic_index" in metafunc.fixturenames:
-        metafunc.parametrize("enum_frontend_asic_index",generate_param_asic_index(metafunc, dut_indices, ASIC_PARAM_TYPE_FRONTEND))
+        metafunc.parametrize("enum_frontend_asic_index",
+                             generate_param_asic_index(metafunc, duts_in_testbed, tbname, dut_indices, ASIC_PARAM_TYPE_FRONTEND))
 
     if "enum_dut_portname" in metafunc.fixturenames:
         metafunc.parametrize("enum_dut_portname", generate_port_lists(metafunc, "all_ports"))