[Webkit-unassigned] [Bug 37262] new-run-webkit-tests should estimate test completion time

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37262





--- Comment #7 from Dirk Pranke <dpranke at chromium.org>  2010-04-08 12:55:54 PST ---
(From update of attachment 52851)
> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/eta.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/eta.py
> new file mode 100644
> index 0000000..ecdf7b5
> --- /dev/null
> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/eta.py
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +import time

I think PEP-8 asks that you put a module-level docstring here.

> +
> +
> +class _Checkpoint(object):
> +    """A reference point for estimating the remaining time."""
> +    def __init__(self, index, total, t):
> +        self._index = index
> +        self._total = total
> +        self._time = t
> +
> +    def estimate(self, index, now):
> +        elapsed_index = index - self._index
> +        elapsed_time = now - self._time
> +        return elapsed_time * (self._total - index) / elapsed_index
> +

Please add docstrings indicating what index, now, total, and t are. 

> +
> +class ETA(object):
> +    """A class for estimating the amount of time remaining.
> +    This class uses an exponentially weighed series of checkpoints to estimate
> +    how much time remains."""

Can you describe why the algorithm is the way it is (or include a reference to
a description of this somewhere, if it's a standard algorithm? I've never seen
it before). It's not obvious why you are skipping some checkpoints, or how this
thing works overall. In particular, it's not clear why this algorithm will
produce better results than a simple (time so far) / ( tests so far / total
tests) estimate, which is a lot simpler and more obvious. I'm sure that this
algorithm is probably better, or else you wouldn't have bothered to code it,
but for anyone not familiar with this type of thing, they'll have no idea how
to debug it or change it.

As to the correctness of the computation, I will not review that at the moment
since I'm hoping you'll explain it to me first :)


In run_webkit_tests.py:

>  
> @@ -714,10 +717,16 @@ class TestRunner:
>  
>      def _display_one_line_progress(self, result_summary):
>          """Displays the progress through the test run."""
> -        percent_complete = 100 * (result_summary.expected + result_summary.unexpected) / result_summary.total
> -        self._meter.update("Testing (%d%%): %d ran as expected, %d didn't, %d left" %
> -            (percent_complete, result_summary.expected,
> -             result_summary.unexpected, result_summary.remaining))
> +        if not self._eta:
> +            self._eta = eta.ETA(result_summary.total)

Should self._eta be created at the start of _run_tests(), rather than here?
Otherwise you are discarding the time spent between starting the test run and
the first invocation of this call.

> +        number_ran = result_summary.expected + result_summary.unexpected
> +        percent_complete = 100 * number_ran / result_summary.total
> +        eta_estimate = self._eta.estimate(number_ran)


> +        eta_string = ", %ds remaining" % math.floor(eta_estimate) if eta_estimate else ""

I'm not at all a fan of this Python idiom (too Perl-y). Can you rewrite this as
a standard if : else: branch? The latter has the advantage of working correctly
with the python coverage tool.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.