Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
301853003:
Make update-flaky-tests to work again and support all bots (Closed)
|
Created:
6 years, 6 months ago by eseidel Modified:
6 years, 6 months ago CC:
blink-reviews, Dirk Pranke Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionMake update-flaky-tests to work again and support all bots
Long ago this was added in hopes of generating a FlakyTests file.
This patch finally finishes that work and adds the FlakyTests
file as well as renames the command to update-flaky-tests
and writes to the file instead of stdout.
TestExpectationLine changed syntax a little since
flaky-tests was written. I've also added support
for specifying path and specifiers values so that
the resulting test output looks right.
I explored having it update TestExpectations directly
(which we're definitely very close to being able to do)
but the consensus was FlakyTests as a separate file would
work better, so lets start with that.
BUG=242366
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175121
Patch Set 1 #Patch Set 2 : Generate lint-passing expectations #Patch Set 3 : Without TestExpectations #Patch Set 4 : Fix test-webkitpy #Patch Set 5 : Update comments #
Total comments: 18
Patch Set 6 : Move to FlakyTests file and make run-webkit-tests use it #Patch Set 7 : Removed use of operator.attrgetter #
Total comments: 1
Patch Set 8 : Remove commented out code and fix test-webkitpy #Patch Set 9 : #Patch Set 10 : #
Messages
Total messages: 46 (0 generated)
Ready for review.
looks pretty good. I've noted a few minor questions ... https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py (right): https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py:139: def __init__(self, results_json, specifiers=None): Can you make specifiers be required and just pass it in from the factory? https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py (right): https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:405: return bool(self.warnings and self.warnings != [TestExpectationParser.MISSING_BUG_WARNING]) why was this needed? https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/port/builders.py (right): https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/port/builders.py:45: "WebKit Win7": {"port_name": "win-win7", "specifiers": ['Win7']}, Should the non-dbg bots have 'Release'? https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/port/builders.py:56: "WebKit Mac10.8 (retina)": {"port_name": "mac-retina", "specifiers": ['MountainLion', 'Retina']}, This one should just be retina. https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/port/builders.py:57: "WebKit Mac10.9": {"port_name": "mac-mavericks", "specifiers": ['Mavericks', 'Retina']}, This one isn't retina. https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... File Tools/Scripts/webkitpy/tool/commands/flakytests.py (right): https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... Tools/Scripts/webkitpy/tool/commands/flakytests.py:48: port_names = filter(lambda name: 'ASAN' not in name, port_names) Perhaps 'ASAN' shouldn't be in all_builder_names() at all? https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... Tools/Scripts/webkitpy/tool/commands/flakytests.py:55: lines.sort(key=operator.attrgetter('path')) Can you use key=lambda line: line.path or something like that instead? https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... Tools/Scripts/webkitpy/tool/commands/flakytests.py:59: # write out the whole TestExpectations file. I don't think we do complain about duplicates across files? (Though I do agree we should filter out duplicates ...)
https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/port/builders.py (right): https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/port/builders.py:44: "WebKit XP": {"port_name": "win-xp", "specifiers": ['XP']}, It's a bummer to hard-code this. Can you add a FIXME to figure out this list from the builder name? https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... File Tools/Scripts/webkitpy/tool/commands/flakytests.py (right): https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... Tools/Scripts/webkitpy/tool/commands/flakytests.py:41: fs = tool.filesystem Nit: this is only used in two places on the same line. It should either be inlined or moved before the line where it's used. https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... Tools/Scripts/webkitpy/tool/commands/flakytests.py:48: port_names = filter(lambda name: 'ASAN' not in name, port_names) On 2014/05/29 18:25:21, Dirk Pranke wrote: > Perhaps 'ASAN' shouldn't be in all_builder_names() at all? +1. I should have done this before when I made rebaseline-o-matic ignore the ASAN bot. Removing the ASAN bot from _exact_matches would have been the better fix. https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... Tools/Scripts/webkitpy/tool/commands/flakytests.py:59: # write out the whole TestExpectations file. On 2014/05/29 18:25:21, Dirk Pranke wrote: > I don't think we do complain about duplicates across files? This is adding to the regular TestExpectations file, so it's not across files.
https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/port/builders.py (right): https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/port/builders.py:44: "WebKit XP": {"port_name": "win-xp", "specifiers": ['XP']}, On 2014/05/29 18:51:20, ojan wrote: > It's a bummer to hard-code this. Can you add a FIXME to figure out this list > from the builder name? I'm not sure that that would be any cleaner. I thought about this change for a while and actually feel pretty good about it as-is. https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... File Tools/Scripts/webkitpy/tool/commands/flakytests.py (right): https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... Tools/Scripts/webkitpy/tool/commands/flakytests.py:59: # write out the whole TestExpectations file. On 2014/05/29 18:51:20, ojan wrote: > On 2014/05/29 18:25:21, Dirk Pranke wrote: > > I don't think we do complain about duplicates across files? > > This is adding to the regular TestExpectations file, so it's not across files. Oh. Right. I suppose the better question is, why wouldn't we keep this in a dedicated FlakyTests file? Mixing hand-edited and automatically generated stuff seems like a bad idea.
https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/port/builders.py (right): https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/port/builders.py:44: "WebKit XP": {"port_name": "win-xp", "specifiers": ['XP']}, On 2014/05/29 18:59:56, Dirk Pranke wrote: > On 2014/05/29 18:51:20, ojan wrote: > > It's a bummer to hard-code this. Can you add a FIXME to figure out this list > > from the builder name? > > I'm not sure that that would be any cleaner. I thought about this change for a > while and actually feel pretty good about it as-is. It's another place where we've duplicated specifier strings. We already have these strings elsewhere in the code (e.g. TestExpectations needs to understand these strings). If we change specifiers, we shouldn't need to change multiple places in the code, e.g. 'Win7' should only exist in one place in the codebase. But yes, I think this is fine for now. I was just asking for a FIXME. https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... File Tools/Scripts/webkitpy/tool/commands/flakytests.py (right): https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... Tools/Scripts/webkitpy/tool/commands/flakytests.py:59: # write out the whole TestExpectations file. On 2014/05/29 18:59:56, Dirk Pranke wrote: > On 2014/05/29 18:51:20, ojan wrote: > > On 2014/05/29 18:25:21, Dirk Pranke wrote: > > > I don't think we do complain about duplicates across files? > > > > This is adding to the regular TestExpectations file, so it's not across files. > > Oh. Right. I suppose the better question is, why wouldn't we keep this in a > dedicated FlakyTests file? Mixing hand-edited and automatically generated stuff > seems like a bad idea. Yes! This would solve almost all my concerns I brought up on the blink-dev thread. If we did this, the only concern I'd have left with turning this one is being more conservative in when we consider a test flaky, which is a ~5 line patch to fix. Happy to help with this part if you need it Eric. With the dedicated FlakyTests file: 1. These lines would never conflict with NeedsRebaseline lines. 2. Tests that stop being flaky would automatically get remove from the file each time we run the script since we'd always overwrite the whole file each time. 3. It would make it so that we could make it a goal to remove all the flaky lines from TestExpectations and only be left with FlakyTests. The only downside to this approach is that it's another file people need to understand for figuring out which tests run where, but that left the barn ages ago when we split up TestExpectations into a dozen files.
I'm glad you agree: https://code.google.com/p/chromium/issues/detail?id=242366#c0
One problem with the FlakyTests file is that you will lose all your bug associations, since right now the flakiness_dashboard uses TestExpectations to hold all its bug -> test information.
I believe I've answered all the review comments. I'm not sure you really want to have "Release" and "Debug" Specified for each builder (or at least for the Release builders) since then folks running in debug mode will just get "expected" failures, but OK.
This is also using the only_show_very_flaky option to BotTestExpectations which should answer Ojan's concern about rebaselines. We can further tweak the algorithm if this is useful.
lgtm. Thanks for working on this. This is very exciting. In a followup patch, we should fix that there are no Debug lines being spit out. There's definitely a bug there. On 2014/05/29 20:00:54, eseidel wrote: > This is also using the only_show_very_flaky option to BotTestExpectations which > should answer Ojan's concern about rebaselines. We can further tweak the > algorithm if this is useful. This is fine for now. In a followup patch, I'd like to see us make only_show_very_flaky a little bit more conservative. But I'm also OK with waiting and seeing how this goes for a bit. On 2014/05/29 19:54:29, eseidel wrote: > One problem with the FlakyTests file is that you will lose all your bug > associations, since right now the flakiness_dashboard uses TestExpectations to > hold all its bug -> test information. This should work fine. It grabs the information off the results json from the buildbot, which will have the aggregated information from all the expectations files. You can see this now by looking up any of the tests that are in NeverFixTests.
Somehow the threading / commenting in this review has broken down, so I'm switching to replying to emails ... sorry :(. On Thu, May 29, 2014 at 12:37 PM, <ojan@chromium.org> wrote: > > https://codereview.chromium.org/301853003/diff/60001/ > Tools/Scripts/webkitpy/layout_tests/port/builders.py > File Tools/Scripts/webkitpy/layout_tests/port/builders.py (right): > > https://codereview.chromium.org/301853003/diff/60001/ > Tools/Scripts/webkitpy/layout_tests/port/builders.py#newcode44 > Tools/Scripts/webkitpy/layout_tests/port/builders.py:44: "WebKit XP": > {"port_name": "win-xp", "specifiers": ['XP']}, > On 2014/05/29 18:59:56, Dirk Pranke wrote: > >> On 2014/05/29 18:51:20, ojan wrote: >> > It's a bummer to hard-code this. Can you add a FIXME to figure out >> > this list > >> > from the builder name? >> > > I'm not sure that that would be any cleaner. I thought about this >> > change for a > >> while and actually feel pretty good about it as-is. >> > > It's another place where we've duplicated specifier strings. > > Ah, good point. Yeah, unfortunately, I think cleaning that up is a decent bit of work. A FIXME for that would be fine. On Thu, May 29, 2014 at 12:39 PM, <eseidel@chromium.org> wrote: > I'm glad you agree: > https://code.google.com/p/chromium/issues/detail?id=242366#c0 > Who is agreeing with what here? I didn't follow this comment ... -- Dirk To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Thu, May 29, 2014 at 1:00 PM, <eseidel@chromium.org> wrote: > I believe I've answered all the review comments. I'm not sure you really > want > to have "Release" and "Debug" Specified for each builder (or at least for > the > Release builders) since then folks running in debug mode will just get > "expected" failures, but OK. > > https://codereview.chromium.org/301853003/ > I don't think I'm following you here. If we're seeing a test be flaky on release but not debug, why would you want that to apply to debug? To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Thanks for the review! I spoke with Dirk in person. I'm not in a huge rush, but would like to land today. Are there any AIs for me before I land?
On 2014/05/29 20:45:08, Dirk Pranke wrote: > On Thu, May 29, 2014 at 1:00 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=eseidel@chromium.org> wrote: > > > I believe I've answered all the review comments. I'm not sure you really > > want > > to have "Release" and "Debug" Specified for each builder (or at least for > > the > > Release builders) since then folks running in debug mode will just get > > "expected" failures, but OK. > > > > https://codereview.chromium.org/301853003/ > > > > I don't think I'm following you here. If we're seeing a test be flaky on > release but not debug, why would you want that to apply to debug? > > To unsubscribe from this group and stop receiving emails from it, send an email > to https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=blink-reviews+unsubscribe@.... My worry was that we had Release coverage but not Debug coverage for bots which developers were using so we'd end up with developers seeing flakes locally on Debug but not Release.
The CQ bit was checked by eseidel@chromium.org
Feel free to un-CQ if you have further comments for me to address.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/301853003/80001
basically lgtm to me as well. I had a couple of nits but those can be addressed later. https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... File Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py (right): https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py:139: def __init__(self, results_json, specifiers=None): On 2014/05/29 18:25:21, Dirk Pranke wrote: > Can you make specifiers be required and just pass it in from the factory? I think you missed this comment? https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... File Tools/Scripts/webkitpy/tool/commands/flakytests.py (right): https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... Tools/Scripts/webkitpy/tool/commands/flakytests.py:55: lines.sort(key=operator.attrgetter('path')) On 2014/05/29 18:25:21, Dirk Pranke wrote: > Can you use key=lambda line: line.path or something like that instead? I think you missed this comment as well ...
On 2014/05/29 21:06:45, Dirk Pranke wrote: > basically lgtm to me as well. I had a couple of nits but those can be addressed > later. > > https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... > File Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py > (right): > > https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/l... > Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py:139: > def __init__(self, results_json, specifiers=None): > On 2014/05/29 18:25:21, Dirk Pranke wrote: > > Can you make specifiers be required and just pass it in from the factory? > > I think you missed this comment? > > https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... > File Tools/Scripts/webkitpy/tool/commands/flakytests.py (right): Sorry, i did. That makes it more testable, but makes the API more awkward. It seems reasonable to me that the only input to the ResultsJSON would be the json itself? > https://codereview.chromium.org/301853003/diff/60001/Tools/Scripts/webkitpy/t... > Tools/Scripts/webkitpy/tool/commands/flakytests.py:55: > lines.sort(key=operator.attrgetter('path')) > On 2014/05/29 18:25:21, Dirk Pranke wrote: > > Can you use key=lambda line: line.path or something like that instead? > > I think you missed this comment as well ... Don't like operator.attrgetter? :) Changed.
The CQ bit was checked by eseidel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/301853003/100001
On Thu, May 29, 2014 at 2:11 PM, <eseidel@chromium.org> wrote: > > https://codereview.chromium.org/301853003/diff/60001/ > Tools/Scripts/webkitpy/tool/commands/flakytests.py > >> File Tools/Scripts/webkitpy/tool/commands/flakytests.py (right): >> > > Sorry, i did. That makes it more testable, but makes the API more > awkward. It > seems reasonable to me that the only input to the ResultsJSON would be the > json > itself? It's not so much that it's more testable, as that I tend to like to avoid optional keyword args where possible (I think it makes the API clearer). In this case, it's not true that the only input (i.e., dependency) to the BotTestExpectations object is the ResultsJSON, since you also need the specifiers; it's just that the specifiers are a static mapping, rather than a dynamic one like the json. Also, BotTestExpectations() itself isn't really a public class, since it's only exposed via the factory object (i.e., you're only changing one caller, who actually has the value sitting right there). At any rate, this is not something worth spending much time on ... your call. > https://codereview.chromium.org/301853003/diff/60001/ > Tools/Scripts/webkitpy/tool/commands/flakytests.py#newcode55 > >> Tools/Scripts/webkitpy/tool/commands/flakytests.py:55: >> lines.sort(key=operator.attrgetter('path')) >> On 2014/05/29 18:25:21, Dirk Pranke wrote: >> > Can you use key=lambda line: line.path or something like that instead? >> > > I think you missed this comment as well ... >> > > Don't like operator.attrgetter? :) Changed. > > Not really a fan of anything in the operator module :). -- Dirk To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Looks like you need to update some webkitpy tests.
https://codereview.chromium.org/301853003/diff/100001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/tool/commands/flakytests.py (right): https://codereview.chromium.org/301853003/diff/100001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/tool/commands/flakytests.py:54: # lines = filter(lambda line: not current_expectations.model().has_test(line.name), lines) Why leave in this comment + the commented out code?
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9455) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9771)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9461)
The CQ bit was checked by eseidel@chromium.org
The irony.
I see, I guess I made webkit_python_tests fail. I thought I had fixed all test-webkipy failures. Investitgatin.
The CQ bit was unchecked by eseidel@chromium.org
On 2014/05/29 22:54:52, eseidel wrote: > I see, I guess I made webkit_python_tests fail. I thought I had fixed all > test-webkipy failures. Investitgatin. While you're uploading a new patch, please remove the commented out code in flakytests.py.
The CQ bit was checked by eseidel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/301853003/110001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
The CQ bit was checked by eseidel@chromium.org
The CQ bit was unchecked by eseidel@chromium.org
The CQ bit was checked by eseidel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/301853003/110001
The CQ bit was checked by eseidel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/301853003/130001
The CQ bit was unchecked by eseidel@chromium.org
The CQ bit was checked by eseidel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eseidel@chromium.org/301853003/130001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9562)
Message was sent while issue was closed.
Change committed as 175121 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
