[Telemetry] Ensure that story display names are unique

telemetry/telemetry/story/story_set.py

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import inspect
 import os
 
 from telemetry.story import story as story_module
 from telemetry.wpr import archive_info
 
@@ skipping 14 matching lines @@
           to self.base_dir.
       cloud_storage_bucket: The cloud storage bucket used to download
           Web Page Replay's archive data. Valid values are: None,
           story.PUBLIC_BUCKET, story.PARTNER_BUCKET, or story.INTERNAL_BUCKET
           (defined in telemetry.util.cloud_storage).
       serving_dirs: A set of paths, relative to self.base_dir, to directories
           containing hash files for non-wpr archive data stored in cloud
           storage.
     """
     self.stories = []
+    self.story_names_and_grouping_keys = set()

[nednguyen, 2016/12/02 19:59:58]

Can you make this private? Things that are not supposed to be touched/modified
by external should be private.

[eakuefner, 2016/12/02 20:28:58]

Done. Also made stories private, since now it's critical that stories and
story_names_and_grouping_keys should remain in sync.

     self._archive_data_file = archive_data_file
     self._wpr_archive_info = None
     archive_info.AssertValidCloudStorageBucket(cloud_storage_bucket)
     self._cloud_storage_bucket = cloud_storage_bucket
     if base_dir:
       if not os.path.isdir(base_dir):
         raise ValueError('Invalid directory path of base_dir: %s' % base_dir)
       self._base_dir = base_dir
     else:
       self._base_dir = os.path.dirname(inspect.getfile(self.__class__))
@@ skipping 46 matching lines @@
   @property
   def wpr_archive_info(self):
     """Lazily constructs wpr_archive_info if it's not set and returns it."""
     if self.archive_data_file and not self._wpr_archive_info:
       self._wpr_archive_info = archive_info.WprArchiveInfo.FromFile(
           os.path.join(self.base_dir, self.archive_data_file), self.bucket)
     return self._wpr_archive_info
 
   def AddStory(self, story):
     assert isinstance(story, story_module.Story)
+    assert self._IsUnique(story), ('Tried to add story with duplicate display '
+                                   'name %s. Story display names should be '
+                                   'unique.' % story.display_name)
     self.stories.append(story)
+    self.story_names_and_grouping_keys.add(
+        story.display_name_and_grouping_key_tuple)
+
+  def _IsUnique(self, story):
+    return (story.display_name_and_grouping_key_tuple not in
+            self.story_names_and_grouping_keys)
 
   def RemoveStory(self, story):
     """Removes a Story.
 
     Allows the stories to be filtered.
     """
     self.stories.remove(story)

[nednguyen, 2016/12/02 20:00:57]

Don't you need to remove the corresponding data from
display_name_and_grouping_key_tuple? Please also add a unittest that would catch
this bug

[eakuefner, 2016/12/02 20:28:58]

Done.

 
   @classmethod
   def Name(cls):
     """Returns the string name of this StorySet.
     Note that this should be a classmethod so the benchmark_runner script can
     match the story class with its name specified in the run command:
     'Run <User story test name> <User story class name>'
     """
     return cls.__module__.split('.')[-1]
 
@@ skipping 27 matching lines @@
     return self.stories.__iter__()
 
   def __len__(self):
     return len(self.stories)
 
   def __getitem__(self, key):
     return self.stories[key]
 
   def __setitem__(self, key, value):
     self.stories[key] = value