Bug 219363 – [build.webkit.org] Add unit-tests based on new buildbot

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Aakash Jain]

[build.webkit.org] Add unit-tests based on new Buildbot. Previous unit-test had very minimal coverage of build steps. Most of the build-steps were not covered. Unit tests are significantly different for old Buildbot (Buildbot 0.8) and newer Buildbot.

Add steps unit-tests for newer buildbot separately from old buildbot unit-tests.

[Aakash Jain]

Created attachment 415044 [details]
WIP

[Aakash Jain]

(In reply to Aakash Jain from comment #1)
> Created attachment 415044 [details]
> WIP
This is not final patch. Need https://bugs.webkit.org/show_bug.cgi?id=219364 to be landed first which will rename steps_unittest.py to steps_unittests_old.py

Then this patch will add steps_unittest.py containing new unit-tests. Also we would be using existing runUnittests.py script (https://trac.webkit.org/browser/webkit/trunk/Tools/CISupport/ews-build/runUnittests.py) to run these unit-tests (both for EWS and build-webkit-org).

[Aakash Jain]

Created attachment 415077 [details]
Patch

[Aakash Jain]

Created attachment 415151 [details]
Patch

[Aakash Jain]

(In reply to Aakash Jain from comment #4)
> Created attachment 415151 [details]
> Patch
This patch ensures that the unit-tests run fine in Python 3.

[Jonathan Bedard]

Comment on attachment 415077 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415077&action=review

> Tools/CISupport/build-webkit-org/steps_unittest.py:199
> +                        timeout=1200,

Since we're starting this from scratch, are we happy with this style? Or do we want:

self.expectRemoteCommands(
    ExpectShell(
        workdir='wkdir',
        timeout=1200,
        logEnviron=True,
        logEnviron=True,
        command=['python', './Tools/Scripts/run-bindings-tests'],
    ) + 0,
)

I'm Ok with the existing style, but it doesn't match what we do outside of buildbot code, and if we want to change things, now is the time.

> Tools/CISupport/build-webkit-org/steps_unittest.py:203
> +            + 0,

I never did understand why we do this "+#" thing after our expect shell commands.

> Tools/CISupport/build-webkit-org/steps_unittest.py:220
> +        return self.runStep()

What motivates returning the result of runStep? That usually is not how Python unit tests work.

[Aakash Jain]

(In reply to Jonathan Bedard from comment #6)
> I'm Ok with the existing style, but it doesn't match what we do outside of  buildbot code, and if we want to change things, now is the time.
Updated patch uses suggested style.
> > Tools/CISupport/build-webkit-org/steps_unittest.py:220
> > +        return self.runStep()
> 
> What motivates returning the result of runStep? That usually is not how Python unit tests work.
This is copied from existing ews-build unit-tests. Note that we aren't using python inbuilt unittest module, we are using "from twisted.trial import unittest".

[Aakash Jain]

Created attachment 415169 [details]
Patch

[Jonathan Bedard]

Comment on attachment 415169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415169&action=review

> Tools/CISupport/build-webkit-org/htdigestparser_unittest.py:39
> +            digest_file.seek(0)

This writes the content at the beginning of the file instead of the end (which was it's previous behavior), is that what we want?

[Aakash Jain]

(In reply to Jonathan Bedard from comment #9)
> This writes the content at the beginning of the file instead of the end (which was it's previous behavior), is that what we want?
It doesn't seems to matter for these unit-tests. They are passing for both python 2 and 3 after this change.

[EWS]

Committed r270354: <https://trac.webkit.org/changeset/270354>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415169 [details].

[Radar WebKit Bug Importer]

<rdar://problem/71902274>