New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 796326 link

Starred by 2 users
Status: Fixed
Owner:
charliea@chromium.org
Closed: Jan 2018
Cc:
bengr@google.com
█ nednguyen@chromium.org
charliea@chromium.org
█ aseemgarg@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----



Sign in to add a comment

"Skip" story expectations without a supplied reason aren't actually skipped

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Dec 19 2017 
Filed by sheriff-o-matic@appspot.gserviceaccount.com on behalf of bengr@google.com

system_health.memory_mobile failing on multiple builders

Builders failed on: 
- health-plan-webview-phone: 
  https://uberchromegw.corp.google.com/i/internal.client.clank/builders/health-plan-webview-phone

Comment 1 by charliea@chromium.org, Dec 19 2017

Owner: charliea@chromium.org
Summary: "Skip" story expectations without a supplied reason aren't actually skipped (was: system_health.memory_mobile failing on multiple builders)
rnephew@ figured out what's going on here, but I'm taking responsibility for the fix. 

The problem is that, in "IsStoryDisabled", we return a "None" value to indicate that the story isn't disabled and we return a string containing the reason to indicate that the story is disabled. 

However, we've now migrated to a new story expectations format where we don't require a reason to disable the story. When a story is disabled without a reason, line 183 in IsStoryDisabled (https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/story/expectations.py?type=cs&q=isstorydisabled&sq=package:chromium&l=187) returns "True", but the "reason" that we return is None, because there is no reason. This means that we use the same return value to indicate that a story should be disabled as the sentinel value that indicates that a story shouldn't be disabled.

I'm going to go ahead and fix this now.
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 19 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/75149e9ea2a082634a59412eade1f15c87688a0d

commit 75149e9ea2a082634a59412eade1f15c87688a0d
Author: Charlie Andrews <charliea@chromium.org>
Date: Tue Dec 19 22:51:17 2017

Fix IsStoryDisabled to not return None when no disable reason exists

None is the same as the sentinel value used to indicate that the story
isn't disabled, which resulted in stories with no disable reason
running even though they should have been disabled.

TBR=nednguyen@chromium.org
R=rnephew@chromium.org
Bug:  chromium:796326 
Change-Id: I5380af797849bc7c796b4f61cdf4e169db5d6532
Reviewed-on: https://chromium-review.googlesource.com/835051
Commit-Queue: Charlie Andrews <charliea@chromium.org>
Reviewed-by: Charlie Andrews <charliea@chromium.org>
Reviewed-by: rnephew <rnephew@chromium.org>

[modify] https://crrev.com/75149e9ea2a082634a59412eade1f15c87688a0d/telemetry/telemetry/story/expectations_unittest.py
[modify] https://crrev.com/75149e9ea2a082634a59412eade1f15c87688a0d/telemetry/telemetry/story/expectations.py

Comment 3 by charliea@chromium.org, Dec 20 2017

Cc: aseemgarg@chromium.org nednguyen@chromium.org charliea@chromium.org
 Issue 796547  has been merged into this issue.

Comment 4 by charliea@google.com, Jan 8 2018

Status: Fixed (was: Available)
►

Sign in to add a comment