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

Issue 900637 link

Starred by 1 user
Status: Fixed
Owner:
chanli@chromium.org
Closed: Nov 12
Cc:
█ lunalu@chromium.org
qyears...@chromium.org
█ nednguyen@chromium.org
robertma@chromium.org
nedngu...@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Layout test failure exposed by removed failure expectations are incorrectly ignored by try bot

Project Member Reported by wangxianzhu@chromium.org, Oct 31 
https://chromium-review.googlesource.com/c/chromium/src/+/1308903 removes failure expectations of some tests, and I expected that it would fail the try bot because the tests do fail. However, the try bot shows green because the failures were incorrectly ignored.

I think it happens as follows:

1. The bot runs layout tests with the patch applied. The tests fail unexpectedly, which is expected.

2. The bot then runs layout tests without the patch. The tests still fail which is expected with the existing failure expectations.

3. The the bot thinks that the tests fail with and without patch, so ignores the failures in the "retry summary" step which results a green build.

We should not ignore unexpected failures in the "with patch" step when the tests fail expectedly in the "without patch" step.

This will cause incorrect removal of failure expectations incorrectly pass the CQ and cause real failures on the waterfall bots.

I'm not sure if this is a recent regression, and set priority 1. If it turns out not a regression, we can lower the priority.

Comment 1 by robertma@chromium.org, Oct 31

Cc: lunalu@chromium.org
lunalu@ reported a similar issue when trying to rebaseline https://crrev.com/c/1299694

I also did some digging yesterday and independently came up with the same conclusion.

I feel like this is a regression, but am not 100% sure. Regardless, it's an important issue that can break both the waterfall and rebaseline-cl.

Where is the logic defined for retrying and ignoring failures?

Comment 2 by robertma@chromium.org, Nov 1

Cc: nedngu...@google.com

Comment 3 by wangxianzhu@chromium.org, Nov 2

Labels: -Type-Bug Type-Bug-Regression
Owner: chanli@chromium.org
Status: Assigned (was: Available)
In https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8931019309027870912/+/steps/webkit_layout_tests__without_patch_/0/logs/json.output/0, the following test is mistakenly treated as an unexpected failure:

      "rendering-contexts.html": {
        "actual": "TEXT TEXT TEXT TEXT TEXT TEXT TEXT TEXT TEXT TEXT",
        "bugs": [
          "Bug(none)"
        ],
        "expected": "FAIL",
        "flag_expectations": [
          "FAIL"
        ],
        "has_stderr": true,
        "text_mismatch": "general text mismatch",
        "time": 0.5
      },

It seems that we fail to match the actual "TEXT" failure to the expected "FAIL" failure at https://chromium.googlesource.com/chromium/tools/build/+blame/HEAD/scripts/slave/recipe_modules/test_utils/util.py#180.

chanli@ can you take a look?

Comment 4 by chanli@chromium.org, Nov 2 

So I think the issue is when counting pass_fail_counts I used the actual and expected as they are and caused this issue.

I will make a quick change on this.
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 6 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/644740bd24fad5e2ab1e798899cc984cf658cbb6

commit 644740bd24fad5e2ab1e798899cc984cf658cbb6
Author: Chan <chanli@chromium.org>
Date: Tue Nov 06 22:14:34 2018

Add check on if a test failure for layout_tests is expected.

When count pass_fail_counts, add a check on failures to see if they are expected or not.

Bug:  900637 
Change-Id: I343b5e7e4b01175c07f7ae1bda5b0721f0929c41
Reviewed-on: https://chromium-review.googlesource.com/c/1316267
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Chan Li <chanli@chromium.org>

[modify] https://crrev.com/644740bd24fad5e2ab1e798899cc984cf658cbb6/scripts/slave/recipe_modules/test_utils/util.py
[modify] https://crrev.com/644740bd24fad5e2ab1e798899cc984cf658cbb6/scripts/slave/recipe_modules/test_utils/test_api.py
[modify] https://crrev.com/644740bd24fad5e2ab1e798899cc984cf658cbb6/scripts/slave/recipe_modules/chromium_tests/tests/steps/swarming_isolated_script_test.py

Comment 6 by raphael....@intel.com, Nov 12 

Can this bug be closed now that the CL above landed?

Comment 7 by chanli@chromium.org, Nov 12

Status: Fixed (was: Assigned)
Judging from https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_layout_tests_slimming_paint_v2/16851, now the bug should be fixed. Closing the bug.
►

Sign in to add a comment