Fix get_latest_build to return actual latest build

Fix get_latest_build to return actual latest build

The code and tests implicitly assume that converting from a list
to a set to a list again does not change ordering. This seems
to be true for small lists, such as the one in the unit tests. For
a list of, say, 200 items, this is not true.

The fix is to add a call to sorted() before indexing the list
of latest cached builds.

BUG=305392
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159201

[mmontgomery, 2013-10-08 22:47:56]

FYI for kbr :)

[ojan, 2013-10-08 22:56:34]

Doh! LGTM


On Tue, Oct 8, 2013 at 3:47 PM, <mmontgomery@chromium.org> wrote:

> Reviewers: ojan,
>
> Message:
> FYI for kbr :)
>
> Description:
> Fix get_latest_build to return actual latest build
>
> The code and tests implicitly assume that converting from a list
> to a set to a list again does not change ordering. This seems
> to be true for small lists, such as the one in the unit tests. For
> a list of, say, 200 items, this is not true.
>
> The fix is to add a call to sorted() before indexing the list
> of latest cached builds.
>
> BUG=305392
>
> Please review this at
https://codereview.chromium.**org/26601003/<https://codereview.chromium.org/2...
>
> SVN Base:
https://chromium.googlesource.**com/chromium/blink.git@master<https://chromiu...
>
> Affected files (+16, -5 lines):
>   M Tools/TestResultServer/**handlers/buildershandler.py
>   M Tools/TestResultServer/**handlers/buildershandler_**unittest.py
>
>
> Index: Tools/TestResultServer/**handlers/buildershandler.py
> diff --git a/Tools/TestResultServer/**handlers/buildershandler.py
> b/Tools/TestResultServer/**handlers/buildershandler.py
> index 966c9c728a373a88776b66d7091c1f**5f00f02660..**
> 1d0bd62c3f0756ba299bc3eb05f792**1d9785b920 100644
> --- a/Tools/TestResultServer/**handlers/buildershandler.py
> +++ b/Tools/TestResultServer/**handlers/buildershandler.py
> @@ -90,7 +90,8 @@ def get_latest_build(build_data):
>
>      latest_cached_builds = set(cached_builds) - set(current_builds)
>      if len(latest_cached_builds) != 0:
> -        latest_build = list(latest_cached_builds)[-1]
> +        latest_cached_builds = sorted(list(latest_cached_**builds))
> +        latest_build = latest_cached_builds[-1]
>      elif len(current_builds) != 0:
>          latest_build = current_builds[0]
>      else:
> Index: Tools/TestResultServer/**handlers/buildershandler_**unittest.py
> diff --git a/Tools/TestResultServer/**handlers/buildershandler_**unittest.py
> b/Tools/TestResultServer/**handlers/buildershandler_**unittest.py
> index 02dbc4cb42d51aafc6a1b65aa33c11**2f6c0abca1..**
> 6a00d7301536e2331494018b65fbf5**023ca416a8 100644
> --- a/Tools/TestResultServer/**handlers/buildershandler_**unittest.py
> +++ b/Tools/TestResultServer/**handlers/buildershandler_**unittest.py
> @@ -50,30 +50,40 @@ class BuildersHandlerTest(unittest.**TestCase):
>                        'currentBuilds': ['3'],
>                        'basedir': 'fake'}
>          latest_build = buildershandler.get_latest_**build(build_data)
> -
>          self.assertEqual(latest_build, '2')
>
>          build_data = {'cachedBuilds': [],
>                        'currentBuilds': ['1', '2', '3'],
>                        'basedir': 'fake'}
>          latest_build = buildershandler.get_latest_**build(build_data)
> -
>          self.assertEqual(latest_build, '1')
>
>          build_data = {'cachedBuilds': ['1', '2', '3'],
>                        'currentBuilds': ['1', '2', '3'],
>                        'basedir': 'fake'}
>          latest_build = buildershandler.get_latest_**build(build_data)
> -
>          self.assertEqual(latest_build, '1')
>
>          build_data = {'cachedBuilds': [],
>                        'currentBuilds': [],
>                        'basedir': 'fake'}
>          latest_build = buildershandler.get_latest_**build(build_data)
> -
>          self.assertEqual(latest_build, None)
>
> +        build_data = {'cachedBuilds': ['1', '2', '3'],
> +                      'currentBuilds': ['3'],
> +                      'basedir': 'fake'}
> +        latest_build = buildershandler.get_latest_**build(build_data)
> +        self.assertEqual(latest_build, '2')
> +
> +        long_list = map(lambda x: str(x), xrange(1000, 1200))
> +        current_build = long_list[-1]
> +        build_data = {'cachedBuilds': long_list[:],
> +                      'currentBuilds': [current_build],
> +                      'basedir': 'fake'}
> +        latest_build = buildershandler.get_latest_**build(build_data)
> +        self.assertEqual(latest_build, long_list[-2])
> +
>      def test_fetch_buildbot_data(self)**:
>          try:
>              fetched_urls = []
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-10-08 22:59:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmontgomery@chromium.org/26601003/4001

[commit-bot: I haz the power, 2013-10-09 05:19:05]

Change committed as 159201