Changes for code review

tests/test_collect.py

@@ -3,18 +3,14 @@
 from pathlib import Path
 from pkgutil import ModuleInfo
 
-from tests.test_util import make_project
-from ward import test, fixture, raises
-from ward.collect import (
-    search_generally,
-    is_test_module,
-    get_module_path,
-    is_excluded_module,
-    remove_excluded_paths,
-    handled_within,
-)
+from ward import fixture, raises, test
+from ward.collect import (get_module_path, handled_within, is_excluded_module,
+                          is_test_module, remove_excluded_paths,
+                          search_generally)
 from ward.testing import Test, each
 
+from tests.test_util import make_project
+
 
 def named():
     assert "fox" == "fox"

ward/rewrite.py

@@ -110,7 +110,7 @@ def rewrite_assertion(test: Test) -> Test:
     # We dedented the code so that it was a valid tree, now re-apply the indent
     for child in ast.walk(new_tree):
         if hasattr(child, "col_offset"):
-            child.col_offset = getattr(child, 'col_offset', 0) + col_offset
+            child.col_offset = getattr(child, "col_offset", 0) + col_offset
 
     # Reconstruct the test function
     new_mod_code_obj = compile(new_tree, code_obj.co_filename, "exec")

ward/run.py

@@ -6,8 +6,12 @@
 import click
 from colorama import init
 from ward._ward_version import __version__
-from ward.collect import (get_info_for_modules, get_tests_in_modules,
-                          load_modules, search_generally)
+from ward.collect import (
+    get_info_for_modules,
+    get_tests_in_modules,
+    load_modules,
+    search_generally,
+)
 from ward.config import set_defaults_from_config
 from ward.rewrite import rewrite_assertions_in_tests
 from ward.suite import Suite
@@ -71,8 +75,7 @@
     help="Look for tests in PATH.",
 )
 @click.option(
-    "-d",
-    "--duration",
+    "--show-slowest",
     type=int,
     help="Record and display duration of n longest running tests",
     default=0,
@@ -88,7 +91,7 @@ def run(
     order: str,
     capture_output: bool,
     config: str,
-    duration: int,
+    show_slowest: int,
 ):
     start_run = default_timer()
     paths = [Path(p) for p in path]
@@ -110,7 +113,7 @@ def run(
         test_results, time_to_collect=time_to_collect, fail_limit=fail_limit
     )
     time_taken = default_timer() - start_run
-    writer.output_test_result_summary(results, time_taken, duration)
+    writer.output_test_result_summary(results, time_taken, show_slowest)
 
     exit_code = get_exit_code(results)
 

ward/suite.py

@@ -38,8 +38,8 @@ def generate_test_runs(self, order="standard") -> Generator[TestResult, None, No
         for test in self.tests:
             generated_tests = test.get_parameterised_instances()
             num_tests_per_module[test.path] -= 1
-            for idx, generated_test in enumerate(generated_tests):
-                yield generated_test.run(self.cache, idx)
+            for generated_test in generated_tests:
+                yield generated_test.run(self.cache)
                 self.cache.teardown_fixtures_for_scope(
                     Scope.Test, scope_key=generated_test.id
                 )

ward/terminal.py

@@ -52,15 +52,10 @@ def format_test_id(test_result: TestResult) -> (str, str):
 
 
 def format_test_location(test_result: TestResult) -> str:
-    """
-    """
-
     return f"{test_result.test.module_name}:{test_result.test.line_number}"
 
 
 def format_test_case_number(test_result: TestResult) -> str:
-    """
-    """
     # If we're executing a parameterised test
     param_meta = test_result.test.param_meta
     if param_meta.group_size > 1:
@@ -406,10 +401,10 @@ def result_checkbox(self, expect):
         return result_marker
 
     def output_test_result_summary(
-        self, test_results: List[TestResult], time_taken: float, duration: int
+        self, test_results: List[TestResult], time_taken: float, show_slowest: int
     ):
-        if duration:
-            self._output_longest_durations(test_results, duration)
+        if show_slowest:
+            self._output_slowest_tests(test_results, show_slowest)
         outcome_counts = self._get_outcome_counts(test_results)
         if test_results:
             chart = self.generate_chart(
@@ -447,16 +442,17 @@ def output_test_result_summary(
 
         print(output)
 
-    def _output_longest_durations(self, test_results: List[TestResult], num_tests: int):
+    def _output_slowest_tests(self, test_results: List[TestResult], num_tests: int):
         test_results = sorted(
             test_results, key=lambda r: r.test.timer.duration, reverse=True
         )
-        print("Longest Running Tests\n")
+        self.print_divider()
+        heading = f"{colored('Longest Running Tests:', color='cyan', attrs=['bold'])}\n"
+        print(indent(heading, INDENT))
         for result in test_results[:num_tests]:
             test_id = format_test_id(result)
-            print(
-                f"{result.test.timer.duration:.2f} sec {test_id} {result.test.description} "
-            )
+            message = f"{result.test.timer.duration:.2f} sec {test_id} {result.test.description} "
+            print(indent(message, DOUBLE_INDENT))
         print()
 
     def output_captured_stderr(self, test_result: TestResult):

ward/testing.py

@@ -106,7 +106,6 @@ class Test:
     serr: StringIO = field(default_factory=StringIO)
     ward_meta: WardMeta = field(default_factory=WardMeta)
     timer: Optional["Timer"] = None
-    args: Optional[Dict[str, Any]] = None
 
     def run(self, cache: FixtureCache, idx: int = 0) -> "TestResult":
 
@@ -117,8 +116,6 @@ def run(self, cache: FixtureCache, idx: int = 0) -> "TestResult":
                 stack.enter_context(redirect_stderr(self.serr))
 
             if isinstance(self.marker, SkipMarker):
-                # TODO:onlyanegg: Do sout and serr need to be part of the class? If they weren't we could have
-                # the ExitStack close them
                 with closing(self.sout), closing(self.serr):
                     result = TestResult(self, TestOutcome.SKIP)
                 return result
@@ -127,9 +124,11 @@ def run(self, cache: FixtureCache, idx: int = 0) -> "TestResult":
                 # TODO:onlyanegg: I don't love this. We're setting up the
                 # fixture within the testing module, but cleaning it up in the
                 # suite module.
-                self.resolve_args(cache, iteration=idx)
-                self.format_description()
-                self.fn(**self.args)
+                resolved_args = self.resolve_args(
+                    cache, iteration=self.param_meta.instance_index
+                )
+                self.format_description(resolved_args)
+                self.fn(**resolved_args)
             except FixtureError as e:
                 outcome = TestOutcome.FAIL
                 error: Optional[Exception] = e
@@ -233,7 +232,7 @@ def get_parameterised_instances(self) -> List["Test"]:
     def deps(self) -> MappingProxyType:
         return inspect.signature(self.fn).parameters
 
-    def resolve_args(self, cache: FixtureCache, iteration: int = 0) -> None:
+    def resolve_args(self, cache: FixtureCache, iteration: int = 0) -> Dict[str, Any]:
         """
         Resolve fixtures and return the resultant name -> Fixture dict.
         If the argument is not a fixture, the raw argument will be used.
@@ -242,9 +241,9 @@ def resolve_args(self, cache: FixtureCache, iteration: int = 0) -> None:
         """
         if self.capture_output:
             with redirect_stdout(self.sout), redirect_stderr(self.serr):
-                self.args = self._resolve_args(cache, iteration)
+                return self._resolve_args(cache, iteration)
         else:
-            self.args = self._resolve_args(cache, iteration)
+            return self._resolve_args(cache, iteration)
 
     def _resolve_args(self, cache: FixtureCache, iteration: int) -> Dict[str, Any]:
         if not self.has_deps:
@@ -385,7 +384,7 @@ def _unpack_resolved(self, fixture_dict: Dict[str, Any]) -> Dict[str, Any]:
                 resolved_vals[k] = arg
         return resolved_vals
 
-    def format_description(self) -> str:
+    def format_description(self, args: Dict[str, Any]) -> str:
         """
         Applies any necessary string formatting to the description,
         given a dictionary `arg_map` of values that will be injected
@@ -395,7 +394,7 @@ def format_description(self) -> str:
         Returns the newly updated description.
         """
 
-        format_dict = FormatDict(**self.args)
+        format_dict = FormatDict(**args)
         if not self.description:
             self.description = ""