1048446 - selftests for Mochitest

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[(not currently active) Ted Mielczarek]

For the xpcshell harness we have a selftest set of tests:
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/selftest.py

These let us write tests for harness behavior in specific scenarios like various kinds of test failures. We should write a similar harness for Mochitest so we can ensure that we don't break various harness features.

Comment 1

[Martijn Wargers (dead)]

2 bugs were test failures aren't caught by the mochitest framework: bug 1032878 and bug 1048423.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: Implement a basic selftest harness for mochitests

This is a work in progress knock off of xpshell's selftests from the long weekend. Some notes:

* Each test method launches the browser, so this is pretty slow. Also the logs are pretty big, so searching lots of them will be slow.
* The leak log stuff makes no sense for opt builds.
* I've probably run afoul of some automation assumptions that would prevent launching the browser over and over during make check.
* These work ok locally on desktop osx and ubuntu.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8482947 [details] [diff] [review]
Implement a basic selftest harness for mochitests

Review of attachment 8482947 [details] [diff] [review]:
-----------------------------------------------------------------

There are definitely a few ugly things here, but this is a great start. We're way better off having this than not having this, certainly! I think given the expected runtime of these tests we should plan ahead for how to run them from the test package. Maybe we can stuff them into mochitest-other?

::: testing/mochitest/mochicheck.py
@@ +16,5 @@
> +from mozbuild.base import MozbuildObject
> +
> +os.environ.pop('MOZ_OBJDIR', None)
> +build_obj = MozbuildObject.from_environment()
> +mozinfo.find_and_update_from_json()

This whole pattern here is kind of confusing. Do we need to load the mozinfo.json? Can we just stub it out? Why exactly do we need to call find_and_update_from_json twice?

@@ +51,5 @@
> +</script>
> +</pre>
> +</body>
> +</html>
> +"""

This might be nicer in a separate file like "test_template.html", but it's not a terribly big deal.

@@ +68,5 @@
> +            import imp
> +            path = os.path.join(mochitest_dir, 'runtests.py')
> +            with open(path, 'r') as fh:
> +                imp.load_module('mochitest', fh, path,
> +                    ('.py', 'r', imp.PY_SOURCE))

Boy, this is terrible.

@@ +139,5 @@
> +                testlines.extend(t[1:])
> +        self.manifest = self.writeFile("mochitest.ini", """
> +[DEFAULT]
> +head =
> +tail =

I don't think Mochitest actually respects head and tail, FWIW.

@@ +181,5 @@
> +    def assertNotInLog(self, s):
> +        """
> +        Assert that the string |s| is not contained in self.stdout.
> +        """
> +        self._assertLog(s, False)

Do we really want to be checking stdout here, or should we be writing tests to check things in the structured logs instead? That feels like the right direction.

@@ +231,5 @@
> +            return leak_file
> +
> +        @leak_report_file.setter
> +        def leak_report_file(cls, val):
> +            pass

This is clever, but I wonder if you can't do the same thing with mock?

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Thinking about this some more and after some discussion perhaps mozharness would be a good place to measure the harness' ability to correctly report failures and invert the failure condition at the very end if necessary for the sake of the final UI. This would, however, constrain the context for how these tests would be run.

Comment 5

[(not currently active) Ted Mielczarek]

I'm not wild about that due to the extra layer necessary. Your patch here is nice in that you can run it like any other set of tests, right from the objdir. Making it harder to run the tests means developers won't catch bustage without pushing.

Comment 6

[Andrew Halberstadt [:ahal]]

I have a patch series that's pretty close to standing up mochitest python tests (using pytest). I hope to finish it up in the week or two.

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1048446 - [python-test] Set up a temporary directory for tests to use,

Review commit: https://reviewboard.mozilla.org/r/118770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118770/

Comment 8

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1048446 - [mozprocess] Allow passing in a function as a value to 'processOutputLine',

Review commit: https://reviewboard.mozilla.org/r/118772/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118772/

Comment 9

[Andrew Halberstadt [:ahal]]

Running mochitest selftests!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ef47b57d989ae34206ac02d3ad9d618a982c6f4&selectedJob=82854091

It's only orange because I added an 'assert False' so pytest would dump the test's stdout to the log. These patches are still pretty rough and hacky.. But the hard part of getting it running is done. Before requesting review, I still need to:

1) Make the installer/tests.zip download and extraction less hacky. Maybe move some logic into mozinstall or something.

2) Make more convenient fixtures in conftest.py. At the very least, the 'runner' fixture needs to automatically save the log to a buffer so the test functions can easily assert against them.

3) Maybe figure out a better way to handle running xvfb.. though I'm mostly satisfied with the current solution.

4) Write a handful of actually useful tests. I want to land at least a couple to act as a reference for anyone who wants to expand the test suite.

Comment 10

[Eric Rahm [:erahm]]

Andrew, have you had any luck moving forward with this?

Comment 11

[Andrew Halberstadt [:ahal]]

I've been on leave the past few months, but returned today and plan on picking this up again.

Comment 12

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

This is a minor convenience for downloading the installer from a url. It uses requests (which should be
available everywhere in-tree). Rather than depending on requests and breaking backwards compatibility,
we only import requests if a url is detected.

Review commit: https://reviewboard.mozilla.org/r/144874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144874/

Comment 13

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1048446 - [mozlog] Don't re-use existing loggers when calling mozlog.commandline.setup_logging a second time,

This is case that got hit with the new mochitest selftest harness. In this scenario, several MochitestDesktop instances
(which call commandline.setup_logging in their constructor) are instantiated in the same interpreter. Because mozlog
implicitly saves the logger state, this meant that setup_logging kept appending duplicate handlers to the existing ones.

I believe that the intent of 'setup_logging' is to get a brand new logger, so it should ensure logger state is reset
on subsequent calls.

Review commit: https://reviewboard.mozilla.org/r/144876/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144876/

Comment 14

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1048446 - [mochitest] Check for a modules dir in the parent directory even if there is a build,

Review commit: https://reviewboard.mozilla.org/r/144878/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144878/

Comment 15

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1048446 - [python-test] Add 'sequential' key to python.ini manifests so tests can opt out of running in parallel,

Review commit: https://reviewboard.mozilla.org/r/144880/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144880/

Comment 16

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1048446 - [python-test] Create a mochitest selftest harness,

This will create a mochitest selftest harness based on |mach python-test|. There
is also a basic test that checks whether TEST-PASS and TEST-UNEXPECTED-FAIL work.

Review commit: https://reviewboard.mozilla.org/r/144882/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144882/

Comment 17

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1048446 - [taskcluster] Move 'require-build' out of run_task and into source_test,

The 'platform' key was recently moved out of 'job' tasks and into the 'source-test' kind. Since
the concept of requiring a build depends on this key, let's move that back to source-test as
well.

Review commit: https://reviewboard.mozilla.org/r/144884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144884/

Comment 18

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1048446 - [taskcluster] Add a 'mochitest selftest' task that depends on a build,

These are integration tests for testing the mochitest framework itself.

Review commit: https://reviewboard.mozilla.org/r/144886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144886/

Comment 19

[Andrew Halberstadt [:ahal]]

Comment on attachment 8845641 [details]
Bug 1048446 - [mozprocess] Allow passing in a function as a value to 'processOutputLine',

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118772/diff/1-2/

Comment 20

[Andrew Halberstadt [:ahal]]

Comment on attachment 8845640 [details]
Bug 1048446 - [python-test] Set up a temporary directory for tests to use,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118770/diff/1-2/

Comment 21

[Andrew Halberstadt [:ahal]]

Here's a working try run (f8 issue has been fixed already):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8cd92da3f0bac01df3fa92e8c7efe23f39aa13d

Note the 'py(mch)' under Linux x64 opt. Also ran builds since they run other python-tests and mochitest-1 as the mochitest harness has changed.

Comment 22

[James Graham [:jgraham]]

Comment on attachment 8873493 [details]
Bug 1048446 - [mozlog] Don't re-use existing loggers when calling mozlog.commandline.setup_logging a second time,

https://reviewboard.mozilla.org/r/144876/#review148828

I guess? I'm slightly nervous about this breaking something, but I take your point that it seems like more reasonable behaviour than what currently happens.

::: testing/mozbase/mozlog/mozlog/commandline.py:219
(Diff revision 1)
>  
>      if not isinstance(logger, StructuredLogger):
>          logger = StructuredLogger(logger)
> +        # The likely intent when using this function is to get a brand new
> +        # logger, so reset state in case it was previously initialized.
> +        logger._state.reset()

Can we make this a method on StructuredLogger rather than having this code reach into private state?

Comment 23

[Geoff Brown [:gbrown]]

Comment on attachment 8845640 [details]
Bug 1048446 - [python-test] Set up a temporary directory for tests to use,

https://reviewboard.mozilla.org/r/118770/#review148834

::: python/mach_commands.py:77
(Diff revision 2)
>          metavar='TEST',
>          help=('Tests to run. Each test can be a single file or a directory. '
>                'Default test resolution relies on PYTHON_UNITTEST_MANIFESTS.'))
> -    def python_test(self,
> +    def python_test(self, *args, **kwargs):
> +        try:
> +            self.tempdir = os.environ['PYTHON_TEST_TMP'] = tempfile.mkdtemp(suffix='-python-test')

Consider s/self.tempdir/tempdir/?

Comment 24

[Geoff Brown [:gbrown]]

Comment on attachment 8873494 [details]
Bug 1048446 - [mochitest] Check for a modules dir in the parent directory even if there is a build,

https://reviewboard.mozilla.org/r/144878/#review148868

Comment 25

[Geoff Brown [:gbrown]]

Comment on attachment 8845641 [details]
Bug 1048446 - [mozprocess] Allow passing in a function as a value to 'processOutputLine',

https://reviewboard.mozilla.org/r/118772/#review148876

Consider unifying your approach with https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/testing/mozbase/mozprocess/mozprocess/processhandler.py#1074

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8873492 [details]
Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

https://reviewboard.mozilla.org/r/144874/#review148920

::: commit-message-62005:5
(Diff revision 1)
> +Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url, r?whimboo
> +
> +This is a minor convenience for downloading the installer from a url. It uses requests (which should be
> +available everywhere in-tree). Rather than depending on requests and breaking backwards compatibility,
> +we only import requests if a url is detected.

Which concerns do you have regarding backward compatibility? If we do a major version bump, it should be all fine.

I'm not that happy about the way you are proposing here. It will also fail if you install the package, without having requests installed separately. So it needs to be added to setup.py.

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py:102
(Diff revision 1)
>      :param dest: Path to install to (to ensure we do not overwrite any existent
>                   files the folder should not exist yet)
>      """
> +
> +    if '://' in src:
> +        return _install_url(src, dest)

This will not work for file:// URLs because there is no adapter for requests.

If we don't want to allow this schema, I would suggest you use `urlparse`, and check `netloc`.

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py:247
(Diff revision 1)
>      mozfile.remove(install_folder)
>  
>  
> +def _install_url(url, dest):
> +    """Saves a url to a temporary file, and passes that through to the
> +    install function.

Please add the parameters.

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py:256
(Diff revision 1)
> +    name = tempfile.mkstemp()[1]
> +    with open(name, 'w+b') as fh:
> +        for chunk in r.iter_content(chunk_size=1024):
> +            fh.write(chunk)
> +    result = install(name, dest)
> +    mozfile.remove(name)

If the download or installation fails, we do not remove the downloaded file. So please wrap the code into try/finally.

Comment 27

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8873497 [details]
Bug 1048446 - [taskcluster] Move 'require-build' out of run_task and into source_test,

https://reviewboard.mozilla.org/r/144884/#review148928

Comment 28

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8873498 [details]
Bug 1048446 - [taskcluster] Add a 'mochitest selftest' task that depends on a build,

https://reviewboard.mozilla.org/r/144886/#review148932

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8873495 [details]
Bug 1048446 - [python-test] Add 'sequential' key to python.ini manifests so tests can opt out of running in parallel,

https://reviewboard.mozilla.org/r/144880/#review149284

this looks good

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8873496 [details]
Bug 1048446 - [python-test] Create a mochitest selftest harness,

https://reviewboard.mozilla.org/r/144882/#review149288

a few nits, comments, questions, concerns...overall this looks like a great framework to help us avoid the missed leaks, crashes that we had in the past.

::: testing/mochitest/tests/python/conftest.py:22
(Diff revision 1)
> +import mozinstall
> +from manifestparser import TestManifest
> +from mozbuild.base import MozbuildObject
> +
> +here = os.path.abspath(os.path.dirname(__file__))
> +build = MozbuildObject.from_environment(cwd=here)

this requires a mach environment, so would this run in a taskcluster task?

::: testing/mochitest/tests/python/conftest.py:60
(Diff revision 1)
> +            r = requests.get(url, stream=True)
> +            with open(bundle, 'w+b') as fh:
> +                for chunk in r.iter_content(chunk_size=1024):
> +                    fh.write(chunk)
> +
> +            mozfile.extract(bundle, dest)

is there existing code in mach to download/extract from GECKO_INSTALLER_URL?  this seems like a good function to already exist or create a generic library.

::: testing/mochitest/tests/python/conftest.py:62
(Diff revision 1)
> +                for chunk in r.iter_content(chunk_size=1024):
> +                    fh.write(chunk)
> +
> +            mozfile.extract(bundle, dest)
> +
> +        return os.path.join(dest, 'mochitest')

what happens if none of the 3 conditions are met?  we should throw some error to the end user

::: testing/mochitest/tests/python/conftest.py:116
(Diff revision 1)
> [e-mail](scope='function')
> +def runtests(setup_harness_root, binary, parser, request):
> +    runtests = pytest.importorskip('runtests')
> +
> +    mochitest_root = runtests.SCRIPT_DIR
> +    test_root = os.path.join(mochitest_root, 'tests', 'selftests')

the files in this patch are in mochitest_root/tests/python/..., is there a selftests/ somewhere else?

::: testing/mochitest/tests/python/conftest.py:141
(Diff revision 1)
> +    def normalize(test):
> +        return {
> +            'name': test,
> +            'relpath': test,
> +            'path': os.path.join(test_root, test),
> +            'manifest': os.path.join(test_root, 'mochitest.ini'),

will this always be mochitest.ini?  in this patch it is called python.ini

::: testing/mochitest/tests/python/conftest.py:153
(Diff revision 1)
> +        manifest.tests.extend(map(normalize, tests))
> +        options['manifestFile'] = manifest
> +        options.update(opts)
> +
> +        result = runtests.run_test_harness(parser, Namespace(**options))
> +        out = json.loads('[' + ','.join(buf.getvalue().splitlines()) + ']')

I feel I am missing the obvious, but where is 'buf' defined?

::: testing/mochitest/tests/python/test_basic_mochitest_plain.py:35
(Diff revision 1)
> +        parser.parse_single_line(json.dumps(line))
> +    return parser.evaluate_parser(status)
> +
> +
> +def test_output_pass(runtests):
> +    status, lines = runtests('test_pass.html')

in reading this patch, I am not clear how the conftest.py::runtest() will find and run 'test_pass.html'.

Comment 31

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873492 [details]
Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

https://reviewboard.mozilla.org/r/144874/#review148920

> Which concerns do you have regarding backward compatibility? If we do a major version bump, it should be all fine.
> 
> I'm not that happy about the way you are proposing here. It will also fail if you install the package, without having requests installed separately. So it needs to be added to setup.py.

No, it would only fail if an attempt to install a url was made since the import happens in the helper function. The reason I'm a bit apprehensive is that this will be the first instance of a mozbase package depending on a 3rd party library.

But seeing as requests is in-tree I guess it should be fine to add to setup.py.

> This will not work for file:// URLs because there is no adapter for requests.
> 
> If we don't want to allow this schema, I would suggest you use `urlparse`, and check `netloc`.

I think in this case I'd prefer to let requests error out so we don't need to worry about which schemes are supported (if it works it works, if it doesn't it doesn't).

I think my updated patch is a good compromise.

Comment 32

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873496 [details]
Bug 1048446 - [python-test] Create a mochitest selftest harness,

https://reviewboard.mozilla.org/r/144882/#review149288

> this requires a mach environment, so would this run in a taskcluster task?

Yes, the taskcluster task is cloning the source and running this via mach so the from_environment() call will work. There won't be a build so e.g trying to call build.get_binary_path() would fail.

> is there existing code in mach to download/extract from GECKO_INSTALLER_URL?  this seems like a good function to already exist or create a generic library.

There is code to download and extract GECKO_INSTALLER_URL in mozinstall (which I added earlier in this patch series), but this is downloading the test package based on GECKO_INSTALLER_URL. I was thinking of adding this to mozinstall too but handling test packages felt a bit like scope bloat there, and even if it wasn't, implementing it properly would require a bit of thought. So in the end I thought it would be better to just put it here for now and potentially find another place for it later.

Do you have any suggestions for where it might live instead? Would filing a follow-up bug be a suitable resolution to this issue?

> what happens if none of the 3 conditions are met?  we should throw some error to the end user

In that case, None is returned and the caller does the error handling. I made this a bit more obvious by explicitly returning None and adding a comment.

> the files in this patch are in mochitest_root/tests/python/..., is there a selftests/ somewhere else?

Yes, the `setup_harness_root` fixture symlinks the `testing/mochitest/tests/python/files` directory to `<mochitest_root>/tests/selftests`. I also anticipate your next comment and added a fallback to `copy` for Windows ;).

> will this always be mochitest.ini?  in this patch it is called python.ini

This is just a dummy file that doesn't exist but is needed because mochitest expects there to be a manifest key in the test object. I'll add a comment.

> I feel I am missing the obvious, but where is 'buf' defined?

It's defined a little higher up in the `runtests` fixture, and the raw structured log gets dumped into it.

> in reading this patch, I am not clear how the conftest.py::runtest() will find and run 'test_pass.html'.

The `runtests` fixture returns a function (called `inner` here). In the `inner` function, the signature is \*tests, \*\*opts, which means a list of tests can be passed in as arguments (and options as keyword arguments). The `inner` function then creates a TestManifest object and appends the normalized tests to it (this is also how the mochitest mach command works). The `normalize` function is what converts the filename to the appropriate test object.

It is a bit magical, but it makes it easier to actually write the tests which I think is the more important thing.

Comment 33

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873492 [details]
Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144874/diff/1-2/

Comment 34

[Andrew Halberstadt [:ahal]]

Comment on attachment 8845641 [details]
Bug 1048446 - [mozprocess] Allow passing in a function as a value to 'processOutputLine',

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118772/diff/2-3/

Comment 35

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873493 [details]
Bug 1048446 - [mozlog] Don't re-use existing loggers when calling mozlog.commandline.setup_logging a second time,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144876/diff/1-2/

Comment 36

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873494 [details]
Bug 1048446 - [mochitest] Check for a modules dir in the parent directory even if there is a build,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144878/diff/1-2/

Comment 37

[Andrew Halberstadt [:ahal]]

Comment on attachment 8845640 [details]
Bug 1048446 - [python-test] Set up a temporary directory for tests to use,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118770/diff/2-3/

Comment 38

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873495 [details]
Bug 1048446 - [python-test] Add 'sequential' key to python.ini manifests so tests can opt out of running in parallel,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144880/diff/1-2/

Comment 39

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873496 [details]
Bug 1048446 - [python-test] Create a mochitest selftest harness,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144882/diff/1-2/

Comment 40

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873497 [details]
Bug 1048446 - [taskcluster] Move 'require-build' out of run_task and into source_test,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144884/diff/1-2/

Comment 41

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873498 [details]
Bug 1048446 - [taskcluster] Add a 'mochitest selftest' task that depends on a build,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144886/diff/1-2/

Comment 42

[James Graham [:jgraham]]

Comment on attachment 8873493 [details]
Bug 1048446 - [mozlog] Don't re-use existing loggers when calling mozlog.commandline.setup_logging a second time,

https://reviewboard.mozilla.org/r/144876/#review149802

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8873492 [details]
Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

https://reviewboard.mozilla.org/r/144874/#review150234

This looks way better. Thanks Andrew.

But I still have some remaining comments. Please see inline. Also could you write at least one unit test which exercises this path? It can even be a URL we cannot reach, or a scheme requests doesn't support. Just to see that nothing is broken with this change. I assume we don't have mozhttpd hooked up for mozbase unit tests?

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py:111
(Diff revision 2)
> +            try:
> +                return _install_url(src, dest)
> +            except:
> +                traceback.print_exc()
> +
>          raise InvalidSource(src + ' is not valid installer file.')

I'm not a fan of only printing the traceback. Lets keep the original stack in the re-raised exception simply by doing something like the following but with the `InvalidSource` exception class:

https://dxr.mozilla.org/mozilla-central/rev/2c6289f56812c30254acfdddabcfec1e149c0336/testing/marionette/client/marionette_driver/marionette.py#1261-1263

It would be good to keep at least the original message, so it's clear what's wrong.

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py:263
(Diff revision 2)
> +    """
> +    r = requests.get(url, stream=True)
> +    name = tempfile.mkstemp()[1]
> +    try:
> +        with open(name, 'w+b') as fh:
> +            for chunk in r.iter_content(chunk_size=1024):

Nowadays we should be faster in downloading when we increase the chunk size. For mozdownload we formerly got the instruction to use 16kB chunks.

Comment 44

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8873496 [details]
Bug 1048446 - [python-test] Create a mochitest selftest harness,

https://reviewboard.mozilla.org/r/144882/#review150276

thanks for the clarification in bugzilla and the code.  I am fine with a followup for downloading a test package and also agree that mozinstall might be overkill.

Comment 45

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873492 [details]
Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

https://reviewboard.mozilla.org/r/144874/#review150234

Sure, I'll add a test for the invalid handler case. Yeah, we don't have an easy way of running a webserver in these tests.

> I'm not a fan of only printing the traceback. Lets keep the original stack in the re-raised exception simply by doing something like the following but with the `InvalidSource` exception class:
> 
> https://dxr.mozilla.org/mozilla-central/rev/2c6289f56812c30254acfdddabcfec1e149c0336/testing/marionette/client/marionette_driver/marionette.py#1261-1263
> 
> It would be good to keep at least the original message, so it's clear what's wrong.

I disagree with changing the exception class of the original exception, that makes it more confusing and you lose the original exception. What's wrong with printing the traceback? InvalidSource still gets raised afterwards.

> Nowadays we should be faster in downloading when we increase the chunk size. For mozdownload we formerly got the instruction to use 16kB chunks.

Good point!

Comment 46

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873492 [details]
Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144874/diff/2-3/

Comment 47

[Andrew Halberstadt [:ahal]]

Comment on attachment 8845641 [details]
Bug 1048446 - [mozprocess] Allow passing in a function as a value to 'processOutputLine',

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118772/diff/3-4/

Comment 48

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873493 [details]
Bug 1048446 - [mozlog] Don't re-use existing loggers when calling mozlog.commandline.setup_logging a second time,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144876/diff/2-3/

Comment 49

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873494 [details]
Bug 1048446 - [mochitest] Check for a modules dir in the parent directory even if there is a build,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144878/diff/2-3/

Comment 50

[Andrew Halberstadt [:ahal]]

Comment on attachment 8845640 [details]
Bug 1048446 - [python-test] Set up a temporary directory for tests to use,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118770/diff/3-4/

Comment 51

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873495 [details]
Bug 1048446 - [python-test] Add 'sequential' key to python.ini manifests so tests can opt out of running in parallel,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144880/diff/2-3/

Comment 52

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873496 [details]
Bug 1048446 - [python-test] Create a mochitest selftest harness,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144882/diff/2-3/

Comment 53

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873497 [details]
Bug 1048446 - [taskcluster] Move 'require-build' out of run_task and into source_test,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144884/diff/2-3/

Comment 54

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873498 [details]
Bug 1048446 - [taskcluster] Add a 'mochitest selftest' task that depends on a build,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144886/diff/2-3/

Comment 55

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8873492 [details]
Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

https://reviewboard.mozilla.org/r/144874/#review150234

> I disagree with changing the exception class of the original exception, that makes it more confusing and you lose the original exception. What's wrong with printing the traceback? InvalidSource still gets raised afterwards.

What do you mean with original exception? The traceback? If yes, then you do not loose it as long as you specify the tb variable when raising the exception again. So in your case it would be:

```
exc, val, tb = sys.exc_info()
raise InvalidSource, "{} is not valid installer file ({}).'.format(src, val), tb
```

This will ensure to raise our own exception type, keeps the original traceback, and only updates the message to be more informative by also including the failure from requests. The version you used, will not keep the traceback!

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8873492 [details]
Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

https://reviewboard.mozilla.org/r/144874/#review150234

> What do you mean with original exception? The traceback? If yes, then you do not loose it as long as you specify the tb variable when raising the exception again. So in your case it would be:
> 
> ```
> exc, val, tb = sys.exc_info()
> raise InvalidSource, "{} is not valid installer file ({}).'.format(src, val), tb
> ```
> 
> This will ensure to raise our own exception type, keeps the original traceback, and only updates the message to be more informative by also including the failure from requests. The version you used, will not keep the traceback!

As given in my example, please use .format(). Otherwise we can get unexpected behavior of str cannot be concatenated with the string.

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8873492 [details]
Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

https://reviewboard.mozilla.org/r/144874/#review151194

Comment 58

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873492 [details]
Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

https://reviewboard.mozilla.org/r/144874/#review150234

> As given in my example, please use .format(). Otherwise we can get unexpected behavior of str cannot be concatenated with the string.

I'm not sure what I'm missing.. my version does all those things including printing the original traceback. How is it lost? But it's ok, I'll do it your way.

Comment 59

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873492 [details]
Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144874/diff/3-4/

Comment 60

[Andrew Halberstadt [:ahal]]

Comment on attachment 8845641 [details]
Bug 1048446 - [mozprocess] Allow passing in a function as a value to 'processOutputLine',

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118772/diff/4-5/

Comment 61

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873493 [details]
Bug 1048446 - [mozlog] Don't re-use existing loggers when calling mozlog.commandline.setup_logging a second time,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144876/diff/3-4/

Comment 62

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873494 [details]
Bug 1048446 - [mochitest] Check for a modules dir in the parent directory even if there is a build,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144878/diff/3-4/

Comment 63

[Andrew Halberstadt [:ahal]]

Comment on attachment 8845640 [details]
Bug 1048446 - [python-test] Set up a temporary directory for tests to use,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118770/diff/4-5/

Comment 64

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873495 [details]
Bug 1048446 - [python-test] Add 'sequential' key to python.ini manifests so tests can opt out of running in parallel,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144880/diff/3-4/

Comment 65

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873496 [details]
Bug 1048446 - [python-test] Create a mochitest selftest harness,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144882/diff/3-4/

Comment 66

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873497 [details]
Bug 1048446 - [taskcluster] Move 'require-build' out of run_task and into source_test,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144884/diff/3-4/

Comment 67

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873498 [details]
Bug 1048446 - [taskcluster] Add a 'mochitest selftest' task that depends on a build,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144886/diff/3-4/

Comment 68

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8873492 [details]
Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

https://reviewboard.mozilla.org/r/144874/#review150234

> I'm not sure what I'm missing.. my version does all those things including printing the original traceback. How is it lost? But it's ok, I'll do it your way.

You only print the traceback but don't carry it on. It means whenever you hit a InvalidSource you will have to manually inspect the full logs to figure out what's wrong, and also the starring on Treeherder will end-up in the same bug, even with different symptoms. 

As long as we can support keeping the details in our own raised exception we should do it.

Comment 69

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8873492 [details]
Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

https://reviewboard.mozilla.org/r/144874/#review151706

Sorry, missed to also hit the finish review button.

Comment 70

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873492 [details]
Bug 1048446 - [mozinstall] Add ability to download and extract installer from a url,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144874/diff/4-5/

Comment 71

[Andrew Halberstadt [:ahal]]

Comment on attachment 8845641 [details]
Bug 1048446 - [mozprocess] Allow passing in a function as a value to 'processOutputLine',

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118772/diff/5-6/

Comment 72

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873493 [details]
Bug 1048446 - [mozlog] Don't re-use existing loggers when calling mozlog.commandline.setup_logging a second time,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144876/diff/4-5/

Comment 73

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873494 [details]
Bug 1048446 - [mochitest] Check for a modules dir in the parent directory even if there is a build,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144878/diff/4-5/

Comment 74

[Andrew Halberstadt [:ahal]]

Comment on attachment 8845640 [details]
Bug 1048446 - [python-test] Set up a temporary directory for tests to use,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118770/diff/5-6/

Comment 75

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873495 [details]
Bug 1048446 - [python-test] Add 'sequential' key to python.ini manifests so tests can opt out of running in parallel,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144880/diff/4-5/

Comment 76

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873496 [details]
Bug 1048446 - [python-test] Create a mochitest selftest harness,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144882/diff/4-5/

Comment 77

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873497 [details]
Bug 1048446 - [taskcluster] Move 'require-build' out of run_task and into source_test,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144884/diff/4-5/

Comment 78

[Andrew Halberstadt [:ahal]]

Comment on attachment 8873498 [details]
Bug 1048446 - [taskcluster] Add a 'mochitest selftest' task that depends on a build,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144886/diff/4-5/

Comment 79

[Andrew Halberstadt [:ahal]]

There was a Windows failure in make check, everything else looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4e07267a76ab04dcc5886528267a950e4250a15

Latest series fixes that and the flake8 errors:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e25cd289ebb7aea30e86078be5d3bcae06e55ec

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42084b6fc3f9
[mozinstall] Add ability to download and extract installer from a url, r=whimboo
https://hg.mozilla.org/integration/autoland/rev/f375a096bff9
[mozprocess] Allow passing in a function as a value to 'processOutputLine', r=gbrown
https://hg.mozilla.org/integration/autoland/rev/525728eff624
[mozlog] Don't re-use existing loggers when calling mozlog.commandline.setup_logging a second time, r=jgraham
https://hg.mozilla.org/integration/autoland/rev/91ce65d749ad
[mochitest] Check for a modules dir in the parent directory even if there is a build, r=gbrown
https://hg.mozilla.org/integration/autoland/rev/455513c2cfe7
[python-test] Set up a temporary directory for tests to use, r=gbrown
https://hg.mozilla.org/integration/autoland/rev/d98f79ccd636
[python-test] Add 'sequential' key to python.ini manifests so tests can opt out of running in parallel, r=jmaher
https://hg.mozilla.org/integration/autoland/rev/83081a324efb
[python-test] Create a mochitest selftest harness, r=jmaher
https://hg.mozilla.org/integration/autoland/rev/b2d113409e21
[taskcluster] Move 'require-build' out of run_task and into source_test, r=dustin
https://hg.mozilla.org/integration/autoland/rev/c7bcf4a7e382
[taskcluster] Add a 'mochitest selftest' task that depends on a build, r=dustin

Comment 81

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/42084b6fc3f9
https://hg.mozilla.org/mozilla-central/rev/f375a096bff9
https://hg.mozilla.org/mozilla-central/rev/525728eff624
https://hg.mozilla.org/mozilla-central/rev/91ce65d749ad
https://hg.mozilla.org/mozilla-central/rev/455513c2cfe7
https://hg.mozilla.org/mozilla-central/rev/d98f79ccd636
https://hg.mozilla.org/mozilla-central/rev/83081a324efb
https://hg.mozilla.org/mozilla-central/rev/b2d113409e21
https://hg.mozilla.org/mozilla-central/rev/c7bcf4a7e382