Bug 1632780 - improve test coverage r=sparky

Added test coverage

Differential Revision: https://phabricator.services.mozilla.com/D72329

python/mozperftest/mozperftest/layers.py

@@ -1,10 +1,10 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
-import logging
+from mozperftest.utils import MachLogger
 
 
-class Layer:
+class Layer(MachLogger):
     # layer name
     name = "unset"
 
@@ -15,9 +15,9 @@ class Layer:
     arguments = {}
 
     def __init__(self, env, mach_command):
+        MachLogger.__init__(self, mach_command)
         self.return_code = 0
         self.mach_cmd = mach_command
-        self.log = mach_command.log
         self.run_process = mach_command.run_process
         self.env = env
 
@@ -43,18 +43,6 @@ class Layer:
     def get_arg(self, name, default=None):
         return self.env.get_arg(name, default, self)
 
-    def info(self, msg, name="mozperftest", **kwargs):
-        self.log(logging.INFO, name, kwargs, msg)
-
-    def debug(self, msg, name="mozperftest", **kwargs):
-        self.log(logging.DEBUG, name, kwargs, msg)
-
-    def warning(self, msg, name="mozperftest", **kwargs):
-        self.log(logging.WARNING, name, kwargs, msg)
-
-    def error(self, msg, name="mozperftest", **kwargs):
-        self.log(logging.ERROR, name, kwargs, msg)
-
     def __enter__(self):
         self.setup()
         return self

python/mozperftest/mozperftest/metadata.py

@@ -1,10 +1,12 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
+from mozperftest.utils import MachLogger
 
 
-class Metadata:
+class Metadata(MachLogger):
     def __init__(self, mach_cmd, env, flavor):
+        MachLogger.__init__(self, mach_cmd)
         self._mach_cmd = mach_cmd
         self.flavor = flavor
         self.browser = {"prefs": {}}

python/mozperftest/mozperftest/metrics/__init__.py

@@ -7,7 +7,7 @@ from mozperftest.metrics.consoleoutput import ConsoleOutput
 
 
 def get_layers():
-    return ConsoleOutput, Perfherder
+    return Perfherder, ConsoleOutput
 
 
 def pick_metrics(env, flavor, mach_cmd):

python/mozperftest/mozperftest/metrics/common.py

@@ -5,112 +5,144 @@ from pathlib import Path
 from mozperftest.metrics.notebook import PerftestNotebook
 
 
-class CommonMetricsSingleton(object):
-    """CommonMetricsSingleton is a metrics class that contains code that is
-    commonly used across all metrics classes.
+class MetricsStorage(object):
+    """Holds data that is commonly used across all metrics layers.
 
-    The metrics classes will be composed of this object, rather than inherit from it,
-    for that reason this class is a singleton. Otherwise, the data would be recomputed
-    for each consecutive metrics processor.
+    An instance of this class represents data for a given and output
+    path and prefix.
     """
 
-    __initialized = False
-    __instance = None
-
-    def __new__(cls, *args, **kw):
-        if not cls.__instance:
-            cls.__instance = object.__new__(cls)
-        return cls.__instance
-
-    def __init__(self, results, warning, output="artifacts", prefix=""):
-        """Initialize CommonMetricsSingleton object.
-
-        :param results list/dict/str: Can be a single path to a result, a
-            list of paths, or a dict containing the data itself.
-        :param output str: Path of where the data will be stored.
-        :param prefix str: Prefix the output files with this string.
-        """
-        if self.__initialized:
-            return
-
+    def __init__(self, output_path, prefix, logger):
         self.prefix = prefix
-        self.output = output
-        self.warning = warning
+        self.output_path = output_path
         self.stddata = None
-
-        p = Path(output)
+        p = Path(output_path)
         p.mkdir(parents=True, exist_ok=True)
+        self.results = []
+        self.logger = logger
 
-        self.results = self.parse_results(results)
-        if not self.results:
-            self.return_code = 1
-            raise Exception("Could not find any results to process.")
-
-        self.__class__.__initialized = True
-
-    def parse_results(self, results):
-        """This function determines the type of results, and processes
-        it accordingly.
-
-        If a single file path is given, the file path is resolved
-        and returned. If a list is given, then all the files
-        in that list (can include directories) are opened and returned.
-        If a dictionary is returned, then nothing will be done to the
-        results, but it will be returned within a list to keep the
-        `self.results` variable type consistent.
-
-        :param results list/dict/str: Path, or list of paths to the data (
-            or the data itself in a dict) of the data to be processed.
-
-        :return list: List of data objects to be processed.
-        """
-        res = []
+    def _parse_results(self, results):
         if isinstance(results, dict):
-            res.append(results)
-        elif isinstance(results, str) or isinstance(results, Path):
+            return [results]
+        res = []
+        # XXX we need to embrace pathlib everywhere.
+        if isinstance(results, (str, Path)):
             # Expecting a single path or a directory
             p = Path(results)
             if not p.exists():
-                self.warning("Given path does not exist: {}".format(results))
+                self.logger.warning("Given path does not exist: {}".format(results))
             elif p.is_dir():
                 files = [f for f in p.glob("**/*.json") if not f.is_dir()]
-                res.extend(self.parse_results(files))
+                res.extend(self._parse_results(files))
             else:
                 res.append(p.as_posix())
-        elif isinstance(results, list):
+        if isinstance(results, list):
             # Expecting a list of paths
             for path in results:
-                res.extend(self.parse_results(path))
+                res.extend(self._parse_results(path))
         return res
 
+    def set_results(self, results):
+        """Processes and sets results provided by the metadata.
+
+        `results` can be a path to a file or a directory. Every
+        file is scanned and we build a list. Alternatively, it
+        can be a mapping containing the results, in that case
+        we just use it direcly, but keep it in a list.
+
+        :param results list/dict/str: Path, or list of paths to the data (
+            or the data itself in a dict) of the data to be processed.
+        """
+        self.results = self._parse_results(results)
+
     def get_standardized_data(
         self, group_name="firefox", transformer="SingleJsonRetriever", overwrite=False
     ):
         """Returns a parsed, standardized results data set.
 
-        If overwrite is True, then we will recompute the results,
-        otherwise, the same dataset will be continuously returned after
-        the first computation. The transformer dictates how the
-        data will be parsed, by default it uses a JSON transformer
-        that flattens the dictionary while merging all the common
-        metrics together.
+        The dataset is computed once then cached unless overwrite is used.
+        The transformer dictates how the data will be parsed, by default it uses
+        a JSON transformer that flattens the dictionary while merging all the
+        common metrics together.
 
         :param group_name str: The name for this results group.
         :param transformer str: The name of the transformer to use
             when parsing the data. Currently, only SingleJsonRetriever
             is available.
+        :param overwrite str: if True, we recompute the results
+        :return dict: Standardized notebook data with containing the
+            requested metrics.
         """
         if not overwrite and self.stddata:
             return self.stddata
 
         # XXX Change config based on settings
         config = {
-            "output": self.output,
+            "output": self.output_path,
             "prefix": self.prefix,
             "customtransformer": transformer,
             "file_groups": {group_name: self.results},
         }
         ptnb = PerftestNotebook(config["file_groups"], config, transformer)
         self.stddata = ptnb.process()
-
         return self.stddata
+
+    def filtered_metrics(
+        self,
+        group_name="firefox",
+        transformer="SingleJsonRetriever",
+        overwrite=False,
+        metrics=None,
+    ):
+
+        """Filters the metrics to only those that were requested by `metrics`.
+
+        If metrics is Falsey (None, empty list, etc.) then no metrics
+        will be filtered. The entries in metrics are pattern matched with
+        the subtests in the standardized data (not a regular expression).
+        For example, if "firstPaint" is in metrics, then all subtests which
+        contain this string in their name, then they will be kept.
+
+        :param metrics list: List of metrics to keep.
+        :return dict: Standardized notebook data with containing the
+            requested metrics.
+        """
+        results = self.get_standardized_data(
+            group_name=group_name, transformer=transformer
+        )["data"]
+
+        if not metrics:
+            return results
+
+        newresults = []
+        for res in results:
+            if any([met in res["subtest"] for met in metrics]):
+                newresults.append(res)
+
+        return newresults
+
+
+_metrics = {}
+
+
+def filtered_metrics(
+    metadata,
+    path,
+    prefix,
+    group_name="firefox",
+    transformer="SingleJsonRetriever",
+    metrics=None,
+):
+    """Returns standardized data extracted from the metadata instance.
+
+    We're caching an instance of MetricsStorage per metrics/storage
+    combination and compute the data only once when this function is called.
+    """
+    key = path, prefix
+    if key not in _metrics:
+        storage = _metrics[key] = MetricsStorage(path, prefix, metadata)
+        storage.set_results(metadata.get_result())
+
+    return storage.filtered_metrics(
+        group_name=group_name, transformer=transformer, metrics=metrics
+    )

python/mozperftest/mozperftest/metrics/consoleoutput.py

@@ -1,9 +1,8 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
+from mozperftest.metrics.common import filtered_metrics
 from mozperftest.layers import Layer
-from mozperftest.metrics.common import CommonMetricsSingleton
-from mozperftest.metrics.utils import filter_metrics
 
 
 class ConsoleOutput(Layer):
@@ -13,34 +12,29 @@ class ConsoleOutput(Layer):
     name = "console"
     activated = False
 
+    arguments = {
+        "metrics": {
+            "nargs": "*",
+            "default": [],
+            "help": "The metrics that should be retrieved from the data.",
+        },
+        # XXX can we guess this by asking the metrics storage ??
+        "prefix": {
+            "type": str,
+            "default": "",
+            "help": "Prefix used by the output files.",
+        },
+    }
+
     def __call__(self, metadata):
-        """Processes the given results into a perfherder-formatted data blob.
-
-        If the `--perfherder` flag isn't provided, then the
-        results won't be processed into a perfherder-data blob. If the
-        flavor is unknown to us, then we assume that it comes from
-        browsertime.
-
-        :param results list/dict/str: Results to process.
-        :param perfherder bool: True if results should be processed
-            into a perfherder-data blob.
-        :param flavor str: The flavor that is being processed.
-        """
-        # Get the common requirements for metrics (i.e. output path,
-        # results to process)
-        cm = CommonMetricsSingleton(
-            metadata.get_result(),
-            self.warning,
-            output=self.get_arg("output"),
-            prefix=self.get_arg("perfherder-prefix"),
+        # Get filtered metrics
+        results = filtered_metrics(
+            metadata,
+            self.get_arg("output"),
+            self.get_arg("prefix"),
+            self.get_arg("metrics"),
         )
-        res = cm.get_standardized_data(
-            group_name="firefox", transformer="SingleJsonRetriever"
-        )
-        _, results = res["file-output"], res["data"]
 
-        # Filter out unwanted metrics
-        results = filter_metrics(results, self.get_arg("perfherder-metrics"))
         if not results:
             self.warning("No results left after filtering")
             return metadata

python/mozperftest/mozperftest/metrics/perfherder.py

@@ -6,8 +6,8 @@ import os
 import statistics
 
 from mozperftest.layers import Layer
-from mozperftest.metrics.common import CommonMetricsSingleton
-from mozperftest.metrics.utils import write_json, filter_metrics
+from mozperftest.metrics.common import filtered_metrics
+from mozperftest.metrics.utils import write_json
 
 
 class Perfherder(Layer):
@@ -45,21 +45,11 @@ class Perfherder(Layer):
             into a perfherder-data blob.
         :param flavor str: The flavor that is being processed.
         """
-        # Get the common requirements for metrics (i.e. output path,
-        # results to process)
-        cm = CommonMetricsSingleton(
-            metadata.get_result(),
-            self.warning,
-            output=self.get_arg("output"),
-            prefix=self.get_arg("perfherder-prefix"),
-        )
-        res = cm.get_standardized_data(
-            group_name="firefox", transformer="SingleJsonRetriever"
-        )
-        _, results = res["file-output"], res["data"]
+        prefix = self.get_arg("prefix")
+        output = self.get_arg("output")
 
-        # Filter out unwanted metrics
-        results = filter_metrics(results, self.get_arg("perfherder-metrics"))
+        # Get filtered metrics
+        results = filtered_metrics(metadata, output, prefix, self.get_arg("metrics"))
         if not results:
             self.warning("No results left after filtering")
             return metadata
@@ -76,16 +66,14 @@ class Perfherder(Layer):
         perfherder_data = self._build_blob(subtests)
 
         file = "perfherder-data.json"
-        if cm.prefix:
-            file = "{}-{}".format(cm.prefix, file)
-        self.info(
-            "Writing perfherder results to {}".format(os.path.join(cm.output, file))
-        )
+        if prefix:
+            file = "{}-{}".format(prefix, file)
+        self.info("Writing perfherder results to {}".format(os.path.join(output, file)))
 
         # XXX "suites" key error occurs when using self.info so a print
         # is being done for now.
         print("PERFHERDER_DATA: " + json.dumps(perfherder_data))
-        metadata.set_output(write_json(perfherder_data, cm.output, file))
+        metadata.set_output(write_json(perfherder_data, output, file))
         return metadata
 
     def _build_blob(

python/mozperftest/mozperftest/metrics/utils.py

@@ -34,28 +34,3 @@ def write_json(data, path, file):
     with open(path, "w+") as f:
         json.dump(data, f)
     return path
-
-
-def filter_metrics(results, metrics):
-    """Filters the metrics to only those that were requested by `metrics`.
-
-    If metrics is Falsey (None, empty list, etc.) then no metrics
-    will be filtered. The entries in metrics are pattern matched with
-    the subtests in the standardized data (not a regular expression).
-    For example, if "firstPaint" is in metrics, then all subtests which
-    contain this string in their name, then they will be kept.
-
-    :param results list: Standardized data from the notebook.
-    :param metrics list: List of metrics to keep.
-    :return dict: Standardized notebook data with containing the
-        requested metrics.
-    """
-    if not metrics:
-        return results
-
-    newresults = []
-    for res in results:
-        if any([met in res["subtest"] for met in metrics]):
-            newresults.append(res)
-
-    return newresults

python/mozperftest/mozperftest/system/proxy.py

@@ -3,7 +3,9 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 import os
 import mozinfo
+
 from mozproxy import get_playback
+from mozproxy.utils import LOG
 from mozperftest.layers import Layer
 
 
@@ -20,6 +22,8 @@ class ProxyRunner(Layer):
     def __init__(self, env, mach_cmd):
         super(ProxyRunner, self).__init__(env, mach_cmd)
         self.proxy = None
+        LOG.info = self.info
+        LOG.error = self.error
 
     def setup(self):
         pass

python/mozperftest/mozperftest/tests/support.py

@@ -9,15 +9,27 @@ from mozperftest.environment import MachEnvironment
 
 
 @contextlib.contextmanager
-def temp_file(name="temp"):
+def temp_file(name="temp", content=None):
     tempdir = tempfile.mkdtemp()
     path = os.path.join(tempdir, name)
+    if content is not None:
+        with open(path, "w") as f:
+            f.write(content)
     try:
         yield path
     finally:
         shutil.rmtree(tempdir)
 
 
+@contextlib.contextmanager
+def temp_dir():
+    tempdir = tempfile.mkdtemp()
+    try:
+        yield tempdir
+    finally:
+        shutil.rmtree(tempdir)
+
+
 def get_running_env(**kwargs):
     from mozbuild.base import MozbuildObject
 

python/mozperftest/mozperftest/tests/test_browsertime.py

@@ -7,6 +7,7 @@ import shutil
 from mozperftest.tests.support import get_running_env
 from mozperftest.environment import BROWSER
 from mozperftest.browser.browsertime import add_options
+from mozperftest.utils import silence
 
 
 HERE = os.path.dirname(__file__)
@@ -30,7 +31,7 @@ def test_browser():
     env.set_arg("tests", [os.path.join(HERE, "example.js")])
 
     try:
-        with browser as b:
+        with browser as b, silence():
             b(metadata)
     finally:
         shutil.rmtree(mach_cmd._mach_context.state_dir)

python/mozperftest/mozperftest/tests/test_consoleoutput.py

@@ -0,0 +1,37 @@
+#!/usr/bin/env python
+import os
+import mozunit
+
+from mozperftest.tests.support import get_running_env, temp_dir
+from mozperftest.environment import METRICS
+from mozperftest.utils import silence
+
+
+HERE = os.path.dirname(__file__)
+
+
+def test_console_output():
+    with temp_dir() as tempdir:
+        options = {
+            "perfherder": True,
+            "perfherder-prefix": "",
+            "console": True,
+            "output": tempdir,
+        }
+        mach_cmd, metadata, env = get_running_env(**options)
+        runs = []
+
+        def _run_process(*args, **kw):
+            runs.append((args, kw))
+
+        mach_cmd.run_process = _run_process
+        metrics = env.layers[METRICS]
+        env.set_arg("tests", [os.path.join(HERE, "example.js")])
+        metadata.set_result(os.path.join(HERE, "browsertime-results"))
+
+        with metrics as console, silence():
+            console(metadata)
+
+
+if __name__ == "__main__":
+    mozunit.main()

python/mozperftest/mozperftest/tests/test_layers.py

@@ -5,7 +5,7 @@ from mozperftest.layers import Layer, Layers
 from mozperftest.environment import MachEnvironment
 
 
-class TestLayer(Layer):
+class _TestLayer(Layer):
     name = "test"
     activated = True
     called = 0
@@ -18,13 +18,13 @@ class TestLayer(Layer):
         self.called += 1
 
 
-class TestLayer2(TestLayer):
+class _TestLayer2(_TestLayer):
     name = "test2"
     activated = True
     arguments = {"arg2": {"type": str, "default": "xxx", "help": "arg2"}}
 
 
-class TestLayer3(TestLayer):
+class _TestLayer3(_TestLayer):
     name = "test3"
     activated = True
 
@@ -33,7 +33,7 @@ def test_layer():
     mach = MagicMock()
     env = MachEnvironment(mach, test=True, test_arg1="ok")
 
-    with TestLayer(env, mach) as layer:
+    with _TestLayer(env, mach) as layer:
         layer.info("info")
         layer.debug("debug")
         assert layer.get_arg("test")
@@ -49,7 +49,7 @@ def test_layer():
 
 def test_layers():
     mach = MagicMock()
-    factories = [TestLayer, TestLayer2, TestLayer3]
+    factories = [_TestLayer, _TestLayer2, _TestLayer3]
     env = MachEnvironment(
         mach, no_test3=True, test_arg1="ok", test2=True, test2_arg2="2"
     )

python/mozperftest/mozperftest/tests/test_mach_commands.py

@@ -18,7 +18,7 @@ from mozperftest.environment import MachEnvironment
 from mozperftest.mach_commands import Perftest
 
 
-class TestMachEnvironment(MachEnvironment):
+class _TestMachEnvironment(MachEnvironment):
     def run(self, metadata):
         return metadata
 
@@ -29,7 +29,7 @@ class TestMachEnvironment(MachEnvironment):
         pass
 
 
-@mock.patch("mozperftest.MachEnvironment", new=TestMachEnvironment)
+@mock.patch("mozperftest.MachEnvironment", new=_TestMachEnvironment)
 def test_command():
     from mozbuild.base import MozbuildObject
 

python/mozperftest/mozperftest/tests/test_metrics_utils.py

@@ -0,0 +1,24 @@
+#!/usr/bin/env python
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+import mozunit
+import json
+
+from mozperftest.metrics.utils import open_file
+from mozperftest.tests.support import temp_file
+
+
+def test_open_file():
+    data = json.dumps({"1": 2})
+
+    with temp_file(name="data.json", content=data) as f:
+        res = open_file(f)
+        assert res == {"1": 2}
+
+    with temp_file(name="data.txt", content="yeah") as f:
+        assert open_file(f) == "yeah"
+
+
+if __name__ == "__main__":
+    mozunit.main()

python/mozperftest/mozperftest/tests/test_proxy.py

@@ -3,14 +3,15 @@ import mozunit
 
 from mozperftest.tests.support import get_running_env
 from mozperftest.environment import SYSTEM
+from mozperftest.utils import silence
 
 
 def test_proxy():
-    mach_cmd, metadata, env = get_running_env()
+    mach_cmd, metadata, env = get_running_env(proxy=True)
     system = env.layers[SYSTEM]
 
     # XXX this will run for real, we need to mock HTTP calls
-    with system as proxy:
+    with system as proxy, silence():
         proxy(metadata)
 
 

python/mozperftest/mozperftest/utils.py

@@ -1,6 +1,7 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
+import logging
 import contextlib
 import sys
 from six import StringIO
@@ -29,3 +30,27 @@ def host_platform():
         return "darwin"
 
     raise ValueError("sys.platform is not yet supported: {}".format(sys.platform))
+
+
+class MachLogger:
+    """Wrapper around the mach logger to make logging simpler.
+    """
+
+    def __init__(self, mach_cmd):
+        self._logger = mach_cmd.log
+
+    @property
+    def log(self):
+        return self._logger
+
+    def info(self, msg, name="mozperftest", **kwargs):
+        self._logger(logging.INFO, name, kwargs, msg)
+
+    def debug(self, msg, name="mozperftest", **kwargs):
+        self._logger(logging.DEBUG, name, kwargs, msg)
+
+    def warning(self, msg, name="mozperftest", **kwargs):
+        self._logger(logging.WARNING, name, kwargs, msg)
+
+    def error(self, msg, name="mozperftest", **kwargs):
+        self._logger(logging.ERROR, name, kwargs, msg)