557336 - Allow UnittestPackagedBuildFactory to be a useful base class

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P1)

Description

[John Ford [:jhford] CET/CEST Berlin Time]

The test running step of the unittest build factory should be abstracted in such a way that other types of tests can use UnittestPackagedBuildFactory as a base class.  This would be useful for mozmill tests and unittests run using maemkit.

Comment 1

[John Ford [:jhford] CET/CEST Berlin Time]

or alternatively create a base class that unittestpackagedbuildfactory derives from.

Comment 2

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: [wip] version 1 of some ideas

Generated from 3c75ccfd152f+ default tip (3c75ccfd152f779043599b8f4877cca519e311b0)

This is a WIP patch.  I haven't tested this at all, not even a checkconfig.

A couple notes about this patch:
-UnittestPackagedBuildFactory has the same __init__ method and should work the same internally (aside from the tests archive downloading and unpacking after the browser and symbols do)
-There is an idea I had for using this new base class to run talos.  I haven't actually looked at what is required for this to work, it is just an idea.
-I have included a couple of ideas for cleaning up the build dir on windows using a built in tool that is the suggested tool (attrib + rmdir)

A couple things I removed from UnittestPackagedBuildFactory:
-ability get filenames implicitly meaning you must do a multi file send change to get tests and/or symbols

A couple notes about the ProposedTalosFactory:
-this class is greatly simplified from TalosFactory
-relies on the same cleanup logic/initial setup logic as the Unittest.  Since they run on the same cluster using roughly the same steps they should prep and clean the same way.

Comment 3

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: [wip] version 1 full file

adding a full file version in case patch bitrots too bad

Comment 4

[Chris AtLee [:catlee]]

Comment on attachment 449812 [details] [diff] [review]
[wip] version 1 of some ideas

>+class MozillaBaseTestBuildFactory(MozillaBuildFactory):
>+
>+    allowed_platforms=['linux', 'linux64', 'win32', 'macosx', 'macosx64']
>+
>     def __init__(self, platform, test_suites, env={}, productName='firefox',
>-            mochitest_leak_threshold=None, crashtest_leak_threshold=None,
>-            totalChunks=None, thisChunk=None, chunkByDir=None, downloadSymbols=True,
>-            **kwargs):
>+                 downloadSymbols=True, **kwargs):
>         self.platform = platform.split('-')[0]
>-
>-        self.env = MozillaEnvironments['%s-unittest' % self.platform].copy()
>-        self.env['MINIDUMP_STACKWALK'] = getPlatformMinidumpPath(self.platform)
>-        self.env.update(env)
>+        assert self.platform in self.__class__.allowed_platforms, \
>+                '%s is not in %s' % (self.platform, self.__class__.allowed_platforms)

No need to reference __class__ here, self.allowed_platforms will work.

Comment 5

[Jeff Hammel]

A very unthorough preliminary review:

>     def __init__(self, platform, test_suites, env={}, productName='firefox',
>                  downloadSymbols=True, **kwargs):
>         self.platform = platform.split('-')[0]
>         assert self.platform in self.__class__.allowed_platforms, \
>                 '%s is not in %s' % (self.platform, self.__class__.allowed_platforms)
>         self.test_suites = test_suites
>         self.env = env

Should pass None as the parameter then `self.env = None or
env.copy()`.  The way it is, if env is not passed and self.env is
updated, the default parameter will be updated on the function (that
is, calling __init__ for a second object will have a modified default:

{{{
> cat tmp.py
class Foo(object):
  def __init__(self, env={}):
    print env
    self.env = env
    self.env[len(self.env)] = 'foo'

Foo()
Foo()
> python tmp.py
{}
{0: 'foo'}
}}}

) and the .copy() prevents the passed in env from being updated.

Also, what is the reason not to use `self.env =
MozillaEnvironments['%s-unittest' % self.platform].copy()` ?


>         self.downloadSymbols

Think you forgot a RHS here?

Overall, I think breaking this into functions as you have done is the
right approach and think this is a good step forward.

What do you mean by the following?

> A couple things I removed from UnittestPackagedBuildFactory:
> -ability get filenames implicitly meaning you must do a multi file
> send change
> to get tests and/or symbols

(Also, what catlee said).

Comment 6

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to comment #4)
> No need to reference __class__ here, self.allowed_platforms will work.
sure, will do


(In reply to comment #5)
> A very unthorough preliminary review:
> Should pass None as the parameter then `self.env = None or
> env.copy()`.  The way it is, if env is not passed and self.env is
> updated, the default parameter will be updated on the function (that
> is, calling __init__ for a second object will have a modified default:

I didn't really look into that, this is more of a braindump and basic structure of a patch.  I will make these changes for the next version of this patch.


> What do you mean by the following?
> 
> > A couple things I removed from UnittestPackagedBuildFactory:
> > -ability get filenames implicitly meaning you must do a multi file
> > send change
> > to get tests and/or symbols

Well, the current logic is (in psuedo-code):

def receive_sendchange():
  if len(sendchange.files) == 1:
    use explicit browser
    guess implicit location for tests
    guess implicit location for symbols
  elif len(sendchange.files) == 2:
    use explicit browser
    use explicit tests
    guess implicit location for symbols
  else:
    use explicit browser
    use explicit tests
    use explicit symbols

The new logic is (for a unittest run, which requires .test.zip with symbols on):
def receive_sendchange():
  if len(sendchange.files) == 1:
    raise DoNotHaveRequiredFilesExc()
  elif len(sendchange.files) == 2:
    raise DoNotHaveRequiredFilesExc()
  else:
    use explicit browser
    use explicit tests
    use explicit symbols

A problem is that if i were to keep the download logic the same as it is now, all classes deriving from this class would try to download the tests archive and wouldn't have the option to do a browser+symbols only sendchange.

Comment 7

[Jeff Hammel]

Attachment: makes a useful abstract base class from the UnittestPackagedBuildFactory

I've written a patch loosely based on :jhford's
(https://bugzilla.mozilla.org/attachment.cgi?id=449812 )

Conceptual differences between this and that patch:

 - I kept the get*_url functions directly as they are from trunk
   buildbotcustom, though I made them methods on the class.  So things
   work as they do now, instead of like
   https://bugzilla.mozilla.org/show_bug.cgi?id=557336#c6

 - MozillaPackagedTestBuildFactory only adds the initial steps; it
   does not have slots (e.g. addRunTests) that are intended to
   subclass from.  Since all the steps are added via a factory's ctor,
   additional steps (such as running the tests) are added by the child
   classes.  Similarly, a addCleanupSteps method is implemented on the
   base class but not called there.

 - I did not port the ProposedTalosFactory to this patch.

I've tested this with an implementation of the MozmillFactory
(https://bugzilla.mozilla.org/attachment.cgi?id=455155 ) and a
buildbot setup as generated by
http://k0s.org/mozilla/hg/TestMozillaBuildbot/ . This works correctly
on Ubuntu 10.06. I have not tested on other platforms nor with other factories.

Comment 8

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 455170 [details] [diff] [review]
makes a useful abstract base class from the UnittestPackagedBuildFactory

I think it would be of greatest utility if this class were to not depend on packaged tests.  I'd like to see this class only be for downloading a browser and optionally symbols.  The reason for this is that mobile talos will be implemented using this class and transfering the packaged tests will be a waste of bandwidth and phone time.

Comment 9

[John Ford [:jhford] CET/CEST Berlin Time]

Will work on this tomorrow.

Comment 10

[Jeff Hammel]

(In reply to comment #8)
> Comment on attachment 455170 [details] [diff] [review]
> makes a useful abstract base class from the UnittestPackagedBuildFactory
> 
> I think it would be of greatest utility if this class were to not depend on
> packaged tests.  I'd like to see this class only be for downloading a browser
> and optionally symbols.  The reason for this is that mobile talos will be
> implemented using this class and transfering the packaged tests will be a waste
> of bandwidth and phone time.

Would it be sufficient to give a constructor argument `download_packaged_tests=False|True` and then switch on that whether this step is added by default? (That and change the class name....to what I'm not sure)

Comment 11

[Jeff Hammel]

Maybe it would be a good idea to get together in person and figure out what to do on a whiteboard.  I'm out today, but other than that, this is my top priority so I would gladly make time to do so

Comment 12

[Jeff Hammel]

Attachment: makes a useful abstract base class from the UnittestPackagedBuildFactory

similar to attachment 455170 [details] [diff] [review] but makes the downloading of the packaged tests optional dependent on a constructor argument

Comment 13

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 461278 [details] [diff] [review]
makes a useful abstract base class from the UnittestPackagedBuildFactory

>+class MozillaPackagedBuildFactory(MozillaBuildFactory):

Maybe we can call this MozillaTestFactory as it won't be exclusively used for Packaged tests.

>+    """
>+    abstract base class for factories using the packaged tests:
>+    - downloads the build (provides: build_filename)
>+    - unpacks the build
>+    - finds the application binary (provides: exepath, exedir)
>+    - gets the build information
>+    - downloads the packaged tests (provides: tests_filename)
>+    - unpacks the tests
>+    - (optional) downloads + unpacks the symbols (provides: symbols_filename)
>+    """

nit: we should update the documentation to reflect the optional status of the downloadPackagedTests item

>+    def __init__(self, platform, env=None, productName='firefox',
>+                 downloadPackagedTests=False, downloadSymbols=True, **kwargs):
>+
>+        self.productName = productName
>         self.platform = platform.split('-')[0]
>-
>-        self.env = MozillaEnvironments['%s-unittest' % self.platform].copy()

As one of the goals of this bug is to remove the assumptions that this class is for running unittests, we should probably move this logic into implementing classes, if we want it.

>-        self.env['MINIDUMP_STACKWALK'] = getPlatformMinidumpPath(self.platform)

>-        self.test_suites = test_suites
>-        self.leak_thresholds = {'mochitest-plain': mochitest_leak_threshold,
>-                                'crashtest': crashtest_leak_threshold}

As above, I think this should be in the implementor classes logic as these are Unittest specific


>+    def addBuild(self):
>+    def addTests(self):
>+    def addSymbols(self):
>+    def wipeBuild(self):

The factories that we use in buildbotcustom follow a naming convention of 'add%sSteps' % group_of_steps_name.  I don't know if this is by design or by accident, but I think we should try to be consistent with what is already in the file and other parts of this class.


>+class UnittestPackagedBuildFactory(MozillaPackagedBuildFactory):
>+
>+    def __init__(self, platform, test_suites, env=None, productName='firefox',
>+                 mochitest_leak_threshold=None, crashtest_leak_threshold=None,
>+                 totalChunks=None, thisChunk=None, chunkByDir=None, downloadSymbols=True,
>+                 **kwargs):
>+
>+        # setup environment
>+        self.platform = platform.split('-')[0]
>+        self.env = MozillaEnvironments['%s-unittest' % self.platform].copy()
>+        self.env['MINIDUMP_STACKWALK'] = getPlatformMinidumpPath(self.platform)
<snip>
>+        # test data
>+        self.test_suites = test_suites
>+        self.leak_thresholds = {'mochitest-plain': mochitest_leak_threshold,
>+                                'crashtest': crashtest_leak_threshold}

Ah, here is the logic in the implementing class.

Comment 14

[John Ford [:jhford] CET/CEST Berlin Time]

I have a patch for this kicking around.  I was going to polish it up today and post it.  Once I have that up, maybe we could see if that meets your needs?

Comment 15

[Jeff Hammel]

Attachment: makes a useful abstract base class from the UnittestPackagedBuildFactory

fixes from jhford's comments

Comment 16

[Jeff Hammel]

(In reply to comment #13)

Comments addressed and uploaded in https://bugzilla.mozilla.org/attachment.cgi?id=461391&action=edit

> Comment on attachment 461278 [details] [diff] [review]
> makes a useful abstract base class from the UnittestPackagedBuildFactory
> 
> >+class MozillaPackagedBuildFactory(MozillaBuildFactory):
> 
> Maybe we can call this MozillaTestFactory as it won't be exclusively used for
> Packaged tests.

Done

> >+    """
> >+    abstract base class for factories using the packaged tests:
> >+    - downloads the build (provides: build_filename)
> >+    - unpacks the build
> >+    - finds the application binary (provides: exepath, exedir)
> >+    - gets the build information
> >+    - downloads the packaged tests (provides: tests_filename)
> >+    - unpacks the tests
> >+    - (optional) downloads + unpacks the symbols (provides: symbols_filename)
> >+    """
> 
> nit: we should update the documentation to reflect the optional status of the
> downloadPackagedTests item

Done.
 
> >+    def __init__(self, platform, env=None, productName='firefox',
> >+                 downloadPackagedTests=False, downloadSymbols=True, **kwargs):
> >+
> >+        self.productName = productName
> >         self.platform = platform.split('-')[0]
> >-
> >-        self.env = MozillaEnvironments['%s-unittest' % self.platform].copy()
> 
> As one of the goals of this bug is to remove the assumptions that this class is
> for running unittests, we should probably move this logic into implementing
> classes, if we want it.

Yes, it is deleted.  See implementing class below.

> >-        self.env['MINIDUMP_STACKWALK'] = getPlatformMinidumpPath(self.platform)
> 
> >-        self.test_suites = test_suites
> >-        self.leak_thresholds = {'mochitest-plain': mochitest_leak_threshold,
> >-                                'crashtest': crashtest_leak_threshold}
> 
> As above, I think this should be in the implementor classes logic as these are
> Unittest specific

Yep, they have been moved below.

> 
> >+    def addBuild(self):
> >+    def addTests(self):
> >+    def addSymbols(self):
> >+    def wipeBuild(self):
> 
> The factories that we use in buildbotcustom follow a naming convention of
> 'add%sSteps' % group_of_steps_name.  I don't know if this is by design or by
> accident, but I think we should try to be consistent with what is already in
> the file and other parts of this class.

I've renamed the 'add' methods 'addDownload%sSteps', as they are mostly about downloading.  I'd be happy to go with another naming convention as desired.

> 
> >+class UnittestPackagedBuildFactory(MozillaPackagedBuildFactory):
> >+
> >+    def __init__(self, platform, test_suites, env=None, productName='firefox',
> >+                 mochitest_leak_threshold=None, crashtest_leak_threshold=None,
> >+                 totalChunks=None, thisChunk=None, chunkByDir=None, downloadSymbols=True,
> >+                 **kwargs):
> >+
> >+        # setup environment
> >+        self.platform = platform.split('-')[0]
> >+        self.env = MozillaEnvironments['%s-unittest' % self.platform].copy()
> >+        self.env['MINIDUMP_STACKWALK'] = getPlatformMinidumpPath(self.platform)
> <snip>
> >+        # test data
> >+        self.test_suites = test_suites
> >+        self.leak_thresholds = {'mochitest-plain': mochitest_leak_threshold,
> >+                                'crashtest': crashtest_leak_threshold}
> 
> Ah, here is the logic in the implementing class.

Yeah, this is the implementor, so all of the implementation-specific logic you referenced should be here, I think.

Comment 17

[Jeff Hammel]

(In reply to comment #14)
> I have a patch for this kicking around.  I was going to polish it up today and
> post it.  Once I have that up, maybe we could see if that meets your needs?

Is your patch functionally much different from this?  If there's anything I'm missing, I'd be happy to try to merge them to get something that meets both of our needs.

Comment 18

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 461391 [details] [diff] [review]
makes a useful abstract base class from the UnittestPackagedBuildFactory

Overall, this patch looks good.  There are a few tweaks that I would like to make before I run it through staging if that is ok with you.

Comment 19

[Jeff Hammel]

(In reply to comment #18)
> Comment on attachment 461391 [details] [diff] [review]
> makes a useful abstract base class from the UnittestPackagedBuildFactory
> 
> Overall, this patch looks good.  There are a few tweaks that I would like to
> make before I run it through staging if that is ok with you.

Yeah, sounds fine.  If you want to talk in person, I'll be in MV tomorrow (or in fact most other days), or feel free to comment here or ping

Comment 20

[John Ford [:jhford] CET/CEST Berlin Time]

I think that using native tools to clobber the testing directory should
probably be split off into its own bug.  I have filed bug 583129 to track that.
 When I run this patch through, I will use the msys rm -rf on windows.

(In reply to comment #19)
> Yeah, sounds fine.  If you want to talk in person, I'll be in MV tomorrow (or
> in fact most other days), or feel free to comment here or ping

I will be out on vacation tomorrow, early next week would work too for me

Comment 21

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: jhford's changes

this changes the organization to make implementing this class for mobile talos/unittests easier.

Comment 22

[Jeff Hammel]

Comments and feedback inline re attachment 467963 [details] [diff] [review]:

diff --git a/process/factory.py b/process/factory.py
--- a/process/factory.py
+++ b/process/factory.py
@@ -6177,8 +6177,23 @@ class MaemoReleaseBuildFactory(MaemoBuil
             workdir=self.objdirAbsPath
         )
 
-class UnittestPackagedBuildFactory(MozillaBuildFactory):
-    def __init__(self, platform, test_suites, env={}, productName='firefox',
-            mochitest_leak_threshold=None, crashtest_leak_threshold=None,
-            totalChunks=None, thisChunk=None, chunkByDir=None, downloadSymbols=True,
-            **kwargs):
+
+def parse_sendchange_files(build, include_substr='', exclude_substrs=[]):
+    '''Given a build object, figure out which files have the include_substr
+    in them, then exclude files that have one of the exclude_substrs. This
+    function uses substring pattern matching instead of regular expressions
+    as it meets the need without incurring as much overhead.'''
+    potential_files=[]
+    for file in build.source.changes[-1].files:
+        if include_substr in file and file not in potential_files:
+            potential_files.append(file)
+    assert len(potential_files) > 0, 'sendchange missing this archive type'
+    for substring in exclude_substrs:
+        for f in potential_files:
+            if substring in f:
+                potential_files.remove(f)
+    assert len(potential_files) == 1, 'Ambiguous testing sendchange!'
+    return potential_files[0]
+
+
+class MozillaTestFactory(MozillaBuildFactory):

No docstring.  Also, this class doesn't have anything to do with tests, so it's kinda a misleading name.

+    def __init__(self, platform, productName='firefox',
+                 downloadSymbols=True, **kwargs):

https://bug557336.bugzilla.mozilla.org/attachment.cgi?id=461391 has a flag to make downloading tests optional (downloadPackagedTests) for this class.  This has been moved to UnittestPackagedBuildFactory with a stub here in `addPrepareExtraArchivesSteps()`.  I'd prefer if this class implemented it (whether or not it was called here or not) so that harnesses could subclass from here directly instead of subclassing from UnittestPackageBuildFactory which is targetted at e.g. mochitests and reftest. But it's not a stopper

         self.platform = platform.split('-')[0]
-
-        self.env = MozillaEnvironments['%s-unittest' % self.platform].copy()
-        self.env['MINIDUMP_STACKWALK'] = getPlatformMinidumpPath(self.platform)
-        self.env.update(env)
         self.productName = productName
+        self.downloadSymbols = downloadSymbols
 
         assert self.platform in getSupportedPlatforms()
 
         MozillaBuildFactory.__init__(self, **kwargs)
 
-        self.test_suites = test_suites
-        self.leak_thresholds = {'mochitest-plain': mochitest_leak_threshold,
-                                'crashtest': crashtest_leak_threshold}
-
-        def get_url(build, include_substr='', exclude_substrs=[]):
-            '''Given a build object, figure out which files have the include_substr
-            in them, then exclude files that have one of the exclude_substrs. This
-            function uses substring pattern matching instead of regular expressions
-            as it meets the need without incurring as much overhead.'''
-            potential_files=[]
-            for file in build.source.changes[-1].files:
-                if include_substr in file and file not in potential_files:
-                    potential_files.append(file)
-            assert len(potential_files) > 0, 'sendchange missing this archive type'
-            for substring in exclude_substrs:
-                for f in potential_files:
-                    if substring in f:
-                        potential_files.remove(f)
-            assert len(potential_files) == 1, 'Ambiguous unittest sendchange!'
-            return potential_files[0]
-
+        self.addCleanupSteps()
+        self.addPrepareBuildSteps()
+        if self.downloadSymbols:
+            self.addPrepareSymbolsSteps()
+        self.addPrepareExtraArchivesSteps()
+        self.addIdentifySteps()
+        self.addSetupSteps()
+        self.addRunTestSteps()
+        self.addTearDownSteps()
+

The control flow should probably be commented on.  What is the rationale for having so many discrete slots here vs. in e.g. addInitialSteps?

+    def addInitialSteps(self):
+        def get_revision(build):
+            try:
+                revision = build.source.changes[-1].revision
+                return revision
+            except:
+                return "not-set"
+        self.addStep(SetBuildProperty(
+         property_name="revision",
+         value=get_revision,
+        ))
+
+        def get_who(build):
+            try:
+                revision = build.source.changes[-1].who
+                return revision
+            except:
+                return "not-set"
+
+        self.addStep(SetBuildProperty(
+         property_name="who",
+         value=get_who,
+        ))
+
+        MozillaBuildFactory.addInitialSteps(self)

So this is effectively the same as my step minus the wipeBuild and comments

+    def addCleanupSteps(self):
+        '''Clean up the relevant places before starting a build'''
+        #On windows, we should try using cmd's attrib and native rmdir
+        self.addStep(ShellCommand(
+            name='rm_builddir',
+            command=['rm', '-rf', 'build'],
+            workdir='.',
+            flunkOnFailure=False, # XXX until bug 558430 is fixed
+        ))

This is much simpler than my wipeBuild function.  Is it sufficient doing this, or, as the comment suggests, should this be expanded for windows?

See http://k0s.org/mozilla/hg/buildbotcustom-patches/file/c0f3c0fbff59/abstract-UnittestPackagedBuildFactory#l385

+    def addPrepareBuildSteps(self):
+        '''This function understands how to prepare a build for having tests run
+        against it.  It downloads, unpacks then sets important properties for use
+        during testing'''
         def get_build_url(build):
              '''Make sure that there is at least one build in the file list'''
             assert len(build.source.changes[-1].files) > 0, 'Unittest sendchange has no files'
-            return get_url(build, exclude_substrs=['.crashreporter-symbols.',
+            return parse_sendchange_files(build, exclude_substrs=['.crashreporter-symbols.',
                                                    '.tests.'])
-
-        def get_tests_url(build):
-            '''If there is only one file, we assume that the tests package is at
-            the same location with the file extension of the browser replaced with
-            .tests.tar.bz2, otherwise we try to find the explicit file'''
-            if len(build.source.changes[-1].files) < 2:
-                build_url = build.getProperty('build_url')
-                for suffix in ('.tar.bz2', '.zip', '.dmg', '.exe'):
-                    if build_url.endswith(suffix):
-                        return build_url[:-len(suffix)] + '.tests.tar.bz2'
-            else:
-                return get_url(build, include_substr='.tests.')
-
-        def get_symbols_url(build):
-            '''If there are two files, we assume that the second file is the tests tarball
-            and use the same location as the build, with the build's file extension replaced
-            with .crashreporter-symbols.zip.  If there are three or more files then we figure
-            out which is the real file'''
-            if len(build.source.changes[-1].files) < 3:
-                build_url = build.getProperty('build_url')
-                for suffix in ('.tar.bz2', '.zip', '.dmg', '.exe'):
-                    if build_url.endswith(suffix):
-                        return build_url[:-len(suffix)] + '.crashreporter-symbols.zip'
-            else:
-                return get_url(build, include_substr='.crashreporter-symbols.')
-
         self.addStep(DownloadFile(
-         url_fn=get_build_url,
-         filename_property='build_filename',
-         url_property='build_url',
-         haltOnFailure=True,
-         name='download build',
+            url_fn=get_build_url,
+            filename_property='build_filename',
+            url_property='build_url',
+            haltOnFailure=True,
+            name='download_build',
         ))

-
-        self.addStep(DownloadFile(
-         url_fn=get_tests_url,
-         filename_property='tests_filename',
-         url_property='tests_url',
-         haltOnFailure=True,
-         name='download tests',
+        self.addStep(UnpackFile(
+            filename=WithProperties('%(build_filename)s'),
+            scripts_dir='../tools/buildfarm/utils',
+            haltOnFailure=True,
+            name='unpack_build',
         ))
-
-        if downloadSymbols:
-            self.addStep(DownloadFile(
-             url_fn=get_symbols_url,
-             filename_property='symbols_filename',
-             url_property='symbols_url',
-             name='download symbols',
-             workdir='build/symbols',
-            ))
-
-        # Unpack the build
-        self.addStep(UnpackFile(
-         filename=WithProperties('%(build_filename)s'),
-         scripts_dir='../tools/buildfarm/utils',
-         haltOnFailure=True,
-         name='unpack build',
-        ))
-
-        # Unpack the tests
-        self.addStep(UnpackFile(
-         filename=WithProperties('%(tests_filename)s'),
-         haltOnFailure=True,
-         name='unpack tests',
-        ))
-
-        if downloadSymbols:
-            # Unpack the symbols
-            self.addStep(UnpackFile(
-             filename=WithProperties('%(symbols_filename)s'),
-             name='unpack symbols',
-             workdir='build/symbols',
-            ))
-
         # Find the application binary!
         if self.platform.startswith('macosx'):
             self.addStep(FindFile(
-             filename="%s-bin" % self.productName,
-             directory=".",
-             filetype="file",
-             max_depth=4,
-             property_name="exepath",
-             name="Find executable",
+                filename="%s-bin" % self.productName,
+                directory=".",
+                filetype="file",
+                max_depth=4,
+                property_name="exepath",
+                name="find_executable",
             ))
         elif self.platform.startswith('win'):
             self.addStep(SetBuildProperty(
@@ -6330,2 +6312,2 @@ class UnittestPackagedBuildFactory(Mozil
              workdir='tools'
             )

The logic to find the binary might be better off in its own function.  It would be even better off as a standalone tool, but i digress.
 
+
+    def addPrepareSymbolsSteps(self):
+        '''This function knows how to setup the symbols for a build to be useful'''
+         def get_symbols_url(build):
+            '''If there are two files, we assume that the second file is the tests tarball
+            and use the same location as the build, with the build's file extension replaced
+            with .crashreporter-symbols.zip.  If there are three or more files then we figure
+            out which is the real file'''
+            if len(build.source.changes[-1].files) < 3:
+                build_url = build.getProperty('build_url')
+                for suffix in ('.tar.bz2', '.zip', '.dmg', '.exe'):
+                    if build_url.endswith(suffix):
+                        return build_url[:-len(suffix)] + '.crashreporter-symbols.zip'
+            else:
+                return parse_sendchange_files(build, include_substr='.crashreporter-symbols.')
+        self.addStep(DownloadFile(
+            url_fn=get_symbols_url,
+            filename_property='symbols_filename',
+            url_property='symbols_url',
+            name='download_symbols',
+            workdir='build/symbols'
+        ))
+        self.addStep(UnpackFile(
+            filename=WithProperties('%(symbols_filename)s'),
+            name='unpack_symbols',
+            workdir='build/symbols'
+        ))
+
+    def addIdentifySteps(self):
+        '''This function knows how to figure out which build this actually is
+        and display it in a useful way'''
         # Figure out build ID and TinderboxPrint revisions
         def get_build_info(rc, stdout, stderr):
             retval = {}
@@ -6346,3 +6359,16 @@ class UnittestPackagedBuildFactory(Mozil
          name='get build info',
         )
 
+
+    def addSetupSteps(self):
+        '''This stub is for implementing classes to do harness specific setup'''
+        pass
+
+    def addRunTestSteps(self):
+        '''This stub is for implementing classes to do the actual test runs'''
+        pass
+
+    def addPrepareExtraArchivesSteps(self):
+        '''This stub is for implementing classes to prepare any extra files that they
+        require'''
+        pass

Instead of adding extra slots for the subclass to implement, why not just have the subclass call `MozillaTestFactory.__init__` and implement whatever logic is subclass-specific after that step? (Also see next comment)

+    def addTearDownSteps(self):
+        if self.buildsBeforeReboot and self.buildsBeforeReboot > 0:
+            self.addPeriodicRebootSteps()

Likewise, if the above is done, how about leaving this function as is but having the subclass call it specifically after all of its steps?

+
+class UnittestPackagedBuildFactory(MozillaTestFactory):
+    def __init__(self, platform, test_suites, env=None, productName='firefox',
+                 mochitest_leak_threshold=None,
+                 crashtest_leak_threshold=None, totalChunks=None,
+                 thisChunk=None, chunkByDir=None, **kwargs):

So, if UnittestPackagedBuildFactory is supposed to be subclassable (which for MozmillFactory, using this layout vs the layout from https://bug557336.bugzilla.mozilla.org/attachment.cgi?id=461391 , it has to be since the methods to download the tests are in UnittestPackagedBuildFactory which contains mozmill), a lot of these arguments are not applicable to generic frameworks.  {mochi,crast}test)_leak_threshold, *chunk* aren't necessary, though since they are optional arguments, its not very important.  test_suites, on the other hand, is a mandatory argument.  Since this is only used in __init__, if I override addRunTestSteps I can just pass in an arbitrary value.  But this is in bad form.  In general, I'd like to see the arguments cleaned up a bit with respect to downstream subclassing.  I'd had to see another level of subclassing, but am not really sure how else to rearchitect this sensibly. (I'm also only vaguely aware of the advantages of this patch over https://bug557336.bugzilla.mozilla.org/attachment.cgi?id=461391 so it is hard to guess what is sufficient to serve all stakeholders.)

+        platform = platform.split('-')[0]
+        self.test_suites = test_suites
+        self.totalChunks = totalChunks
+        self.thisChunk = thisChunk
+        self.chunkByDir = chunkByDir
+        self.env = MozillaEnvironments['%s-unittest' % platform].copy()
+        self.env['MINIDUMP_STACKWALK'] = getPlatformMinidumpPath(platform)
+        self.env.update(env)

Should be `self.env.update(env or {})` as `self.env.update(None)` won't work

+        self.leak_thresholds = {'mochitest-plain': mochitest_leak_threshold,
+                                'crashtest': crashtest_leak_threshold,}
+        MozillaTestFactory.__init__(self, platform, productName, **kwargs)
+
+    def addPrepareExtraArchivesSteps(self):
+        def get_tests_url(build):
+            '''If there is only one file, we assume that the tests package is at
+            the same location with the file extension of the browser replaced with
+            .tests.tar.bz2, otherwise we try to find the explicit file'''
+            if len(build.source.changes[-1].files) < 2:
+                build_url = build.getProperty('build_url')
+                for suffix in ('.tar.bz2', '.zip', '.dmg', '.exe'):
+                    if build_url.endswith(suffix):
+                        return build_url[:-len(suffix)] + '.tests.tar.bz2'

This should definitely be '.tests.zip' . (Likewise, in the comment)

+            else:
+                return parse_sendchange_files(build, include_substr='.tests.')
+        self.addStep(DownloadFile(
+            url_fn=get_tests_url,
+            filename_property='tests_filename',
+            url_property='tests_url',
+            haltOnFailure=True,
+            name='download tests',
+        ))
+        # Unpack the tests
+        self.addStep(UnpackFile(
+            filename=WithProperties('%(tests_filename)s'),
+            haltOnFailure=True,
+            name='unpack tests',
+        ))
+
+    def addRunTestSteps(self):
         # Run them!
         for suite in self.test_suites:
             leak_threshold = self.leak_thresholds.get(suite, None)
@@ -6356,15 +6432,15 @@ class UnittestPackagedBuildFactory(Mozil
                  env=self.env,
                  symbols_path='symbols',
                  leakThreshold=leak_threshold,
-                 chunkByDir=chunkByDir,
-                 totalChunks=totalChunks,
-                 thisChunk=thisChunk,
+                 chunkByDir=self.chunkByDir,
+                 totalChunks=self.totalChunks,
+                 thisChunk=self.thisChunk,
                  maxTime=90*60, # One and a half hours, to allow for slow minis
                 ))
             elif suite == 'xpcshell':
                 self.addStep(unittest_steps.MozillaPackagedXPCShellTests(
                  env=self.env,
-                 platform=platform,
+                 platform=self.platform,
                  symbols_path='symbols',
                  maxTime=120*60, # Two Hours
                 ))
@@ -6380,3 +6456,3 @@ class UnittestPackagedBuildFactory(Mozil
                  maxTime=2*60*60, # Two Hours
                 ))
 
So I can override this method to run the mozmill tests, if we go with this setup. See https://bugzilla.mozilla.org/show_bug.cgi?id=516984

-        if self.buildsBeforeReboot and self.buildsBeforeReboot > 0:
-            self.addPeriodicRebootSteps()
-
-    def addInitialSteps(self):
-        # XX remove flunkOnFailure after bug 558430 is fixed
-        self.addStep(ShellCommand,
-         name='rm_builddir',
-         command=['rm', '-rf', 'build'],
-         workdir='.',
-         flunkOnFailure=False,
-        )
-
-        def get_revision(build):
-            try:
-                revision = build.source.changes[-1].revision
-                return revision
-            except:
-                return "not-set"
-
-        self.addStep(SetBuildProperty(
-         property_name="revision",
-         value=get_revision,
-        ))
-
-        def get_who(build):
-            try:
-                revision = build.source.changes[-1].who
-                return revision
-            except:
-                return "not-set"
-
-        self.addStep(SetBuildProperty(
-         property_name="who",
-         value=get_who,
-        ))
-
-        MozillaBuildFactory.addInitialSteps(self)
 
 class TalosFactory(BuildFactory):
     extName = 'addon.xpi'

Again, I would prefer less of a slotted approach, but this is fine for my purposes, which is chiefly bug 516984 at the moment.

Comment 23

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to comment #22)
> +        self.addCleanupSteps()
> +        self.addPrepareBuildSteps()
> +        if self.downloadSymbols:
> +            self.addPrepareSymbolsSteps()
> +        self.addPrepareExtraArchivesSteps()
> +        self.addIdentifySteps()
> +        self.addSetupSteps()
> +        self.addRunTestSteps()
> +        self.addTearDownSteps()
> +
> 
> The control flow should probably be commented on.  What is the rationale for
> having so many discrete slots here vs. in e.g. addInitialSteps?

I would like to use this for mobile tests as well.  When I do that, I want the only thing that changes to be how the tests are invoked.  I don't want to have to redefine the constructor logic.  As well, this approach is consistent with how other factories in factory.py are written.

> This is much simpler than my wipeBuild function.  Is it sufficient doing this,
> or, as the comment suggests, should this be expanded for windows?

As mentioned in comment 20, I have split changing the way we delete files into another bug.

> The logic to find the binary might be better off in its own function.  It would
> be even better off as a standalone tool, but i digress.

Yep, this bug is about the structure of the class not its functionality :)

> +    def addPrepareExtraArchivesSteps(self):
> +        '''This stub is for implementing classes to prepare any extra files
> that they
> +        require'''
> +        pass
> 
> Instead of adding extra slots for the subclass to implement, why not just have
> the subclass call `MozillaTestFactory.__init__` and implement whatever logic is
> subclass-specific after that step? (Also see next comment)
<snip>
> So, if UnittestPackagedBuildFactory is supposed to be subclassable (which for
> MozmillFactory, using this layout vs the layout from
> https://bug557336.bugzilla.mozilla.org/attachment.cgi?id=461391 , it has to be
> since the methods to download the tests are in UnittestPackagedBuildFactory
> which contains mozmill), a lot of these arguments are not applicable to generic
> frameworks.  {mochi,crast}test)_leak_threshold, *chunk* aren't necessary,
> though since they are optional arguments, its not very important.  test_suites,
> on the other hand, is a mandatory argument.  Since this is only used in
> __init__, if I override addRunTestSteps I can just pass in an arbitrary value. 
> But this is in bad form.  In general, I'd like to see the arguments cleaned up
> a bit with respect to downstream subclassing.  I'd had to see another level of
> subclassing, but am not really sure how else to rearchitect this sensibly. (I'm
> also only vaguely aware of the advantages of this patch over
> https://bug557336.bugzilla.mozilla.org/attachment.cgi?id=461391 so it is hard
> to guess what is sufficient to serve all stakeholders.)

I don't want to change the method signature wherever possible.  I think a possible solution to this would be to go with your idea of having the tests archive download/setup steps in the base class and have UnittestPackagedBuildFactory as an implementing class instead of having it as another base class.

> +        platform = platform.split('-')[0]
> +        self.test_suites = test_suites
> +        self.totalChunks = totalChunks
> +        self.thisChunk = thisChunk
> +        self.chunkByDir = chunkByDir
> +        self.env = MozillaEnvironments['%s-unittest' % platform].copy()
> +        self.env['MINIDUMP_STACKWALK'] = getPlatformMinidumpPath(platform)
> +        self.env.update(env)
> 
> Should be `self.env.update(env or {})` as `self.env.update(None)` won't work

As this is the code that is currently in place, it might be that we want to require an environment be defined in buildbotcustom.env.MozillaEnvironments.  As I am unsure of the original intent, I would like to leave the existing code as is.  I would say that if this is desired behaviour, we should probably be more explicit by commenting or by having an assertion.

> +        self.leak_thresholds = {'mochitest-plain': mochitest_leak_threshold,
> +                                'crashtest': crashtest_leak_threshold,}
> +        MozillaTestFactory.__init__(self, platform, productName, **kwargs)
> +
> +    def addPrepareExtraArchivesSteps(self):
> +        def get_tests_url(build):
> +            '''If there is only one file, we assume that the tests package is
> at
> +            the same location with the file extension of the browser replaced
> with
> +            .tests.tar.bz2, otherwise we try to find the explicit file'''
> +            if len(build.source.changes[-1].files) < 2:
> +                build_url = build.getProperty('build_url')
> +                for suffix in ('.tar.bz2', '.zip', '.dmg', '.exe'):
> +                    if build_url.endswith(suffix):
> +                        return build_url[:-len(suffix)] + '.tests.tar.bz2'
> 
> This should definitely be '.tests.zip' . (Likewise, in the comment)

No, it definitely should not be.  While we currently require an explicit file for the tests in the sendchange, the default mozilla build system behaviour generates .tests.tar.bz2.  Old branches and other projects that haven't taken the patch to change to zip files are also likely not using the explicit second file in the sendchange.  Keeping the default as '.tar.bz2' was a requirement for the change to zip files.

My preference would be to remove the default altogether and require the second file in the sendchange over changing the default.

>  class TalosFactory(BuildFactory):
>      extName = 'addon.xpi'
> 
> Again, I would prefer less of a slotted approach, but this is fine for my
> purposes, which is chiefly bug 516984 at the moment.

I will move the test downloading into this base class as an optional task.  

Also, in replies to a patch, I won't object to removing parts of the patch that aren't directly being commented on :)

Comment 24

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: buildbotcustom patch

This patch moves the majority of the logic that can be reused for other test types into a new base class 'MozillaTestFactory'.  The __init__ method of UnittestPackagedBuildFactory remains unchanged to minimize impact.  The motivation of this patch is to create a base class that can be used for other classes that do similar things (i.e. download a build, unpack it, run tests then reboot).  The first implementation of this class (other than the one included in this patch) will be Mozmill followed soon after with mobile talos and unittests.

This patch does change the ordering of when files are downloaded and unpacked.  Instead of downloading all files, then extracting all files, this patch will do a download then unpack for each.

i.e.
OLD:
for file in files:
  download(file)
for file in files:
  unpack(file)
NEW:
for file in files:
  download(file)
  unpack(file)

The only case that this would make a difference is the failure case where a file cannot be downloaded or is not on the mirror.  In this case, some time will be spent unpacking files that aren't really needed because of a download failure.  My hope is that my work in bug 578752 (once I start) will replace this logic in a more error-resilient way.

I have tested this on stm01 and found that I got production-matching results on every test run that I did.  I tested fed,fed64,leopard,snow,w7 but not w764

Comment 25

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: ideas for a TalosFactory that uses MozillaTestFactory

This is an untested proof of concept for using the new MozillaBuildFactory to run Talos tests.  This patch was designed to not modify the __init__ signature.  It is also designed to be as transparent as possible.

A couple of main points:
  -We need to make sure that cleanup works properly (i.e. bug 583129)
  -addons testing wasn't brought over fully
  -slavebuilddir is used instead of 'workdirBase = "../talos-data"'.  This has
   the same effect but ensures that we don't have to do a lot of workdir
   manipulation every step
  -using the default directory of 'build' might cause issues,
    -workdir="talos" might need to be "build/talos"
    -might need to change harness config file settings for this change

Comment 26

[Chris AtLee [:catlee]]

Comment on attachment 470849 [details] [diff] [review]
ideas for a TalosFactory that uses MozillaTestFactory

Looks good as a POC.  For a final version we should make the __init__ signature match more closely what the base class expects, and get rid of mapOSToPlatform.

Comment 27

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: builder_list.py differences

Beyond the reordering of commands, I noticed that MozillaPackagedXPCShellTests is being given a platform of 'win32' instead of 'win32-debug'.  This is because in the old class, platform=platform was used [1].  This is because the UnittestPackagedBuildFactory.__init__ argument was being used and did not respect the trimming of -debug [2] for self.platform.  Looking at the code for MozillaPackagedXPCShellTests [3], it looks like this is an obsolete argument as it isn't used in that class and is not present in other classes which derive from the same base class.  After a bit of research, I found that this is part of a (seemingly) backed out patch [4] and was only used to do startswith('win') which is valid.  Not sure If i should remove this argument or not.

The diff shows SetBuildProperty differences but that is to do with the moving of rm -rf build.


[1] http://hg.mozilla.org/build/buildbotcustom/file/da7c7585a716/process/factory.py#l6171
[2] http://hg.mozilla.org/build/buildbotcustom/file/da7c7585a716/process/factory.py#l5989
[3] http://hg.mozilla.org/build/buildbotcustom/file/6f0b1cabdec5/steps/unittest.py#l439
[4] http://hg.mozilla.org/build/buildbotcustom/rev/1549a1120883

Comment 28

[Chris AtLee [:catlee]]

Comment on attachment 470505 [details] [diff] [review]
buildbotcustom patch

Looks good, with the fix for the 'for f in potential_files:'

Comment 29

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 470505 [details] [diff] [review]
buildbotcustom patch

http://hg.mozilla.org/build/buildbotcustom/rev/ee1de63652a8