The multi load capability added in https://bugs.webkit.org/show_bug.cgi?id=48233 currently needs some additional test expectations as not all tests works when loaded several times. This is not an issue with the way it is done in DumpRenderTree or test_shell, as the same errors happen when the Python runner sends the same URLs several times.
Created attachment 72326 [details] Patch
Attachment 72326 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:265: trailing whitespace [pep8/W291] [5] Traceback (most recent call last): File "WebKitTools/Scripts/check-webkit-style", line 133, in <module> main() File "WebKitTools/Scripts/check-webkit-style", line 120, in main patch_checker.check(patch) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/patchreader.py", line 66, in check self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/filereader.py", line 128, in process_file self._processor.process(lines, file_path, **kwargs) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 722, in process checker.check(lines) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py", line 117, in check overrides = self._port_obj.test_expectations_overrides() File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py", line 274, in test_expectations_overrides if self._options.multiple_loads is not None and self._options.multiple_loads > 0: AttributeError: 'ChromiumOptions' object has no attribute 'multiple_loads' If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 72326 [details] Patch Looks like this patch would destroy the check-webkit-style.
Created attachment 72491 [details] Patch Fixed style issues in previous patch
With the style comments addressed does anyone have time to take a look?
It would be good to have dpranke review this patch since he knows this code best. I'm slightly worried that our test expectation complexity is growing geometrically. Is there a plan to remove some of these files? Who's going to maintain them? Is there a bot that goes red when they're wrong?
(In reply to comment #6) > It would be good to have dpranke review this patch since he knows this code best. > > I'm slightly worried that our test expectation complexity is growing geometrically. Is there a plan to remove some of these files? Who's going to maintain them? Is there a bot that goes red when they're wrong? Søren, it's probably easiest if you copy me on these patches first. There's three sets of comments here. First, to Adam's concerns, yes, I'm concerned about them as well. I hope to modify the test_expectations file syntax to support generic modifiers to help address this. That doesn't directly address the combinatorial explosion of test configurations, however. Second, to the main thrust of the patch, would it be acceptable to just mark these as flaky tests in the main file, rather than creating a separate file with non-flaky results? E.g., instead of having BUG_ML : fast/images/animated-svg-as-image.html = IMAGE in the ml_expectations.txt file, you could just mark: BUG_ML : fast/images/animated-svg-as-image.html = IMAGE PASS in the main file. This isn't ideal, since like all flaky test expectations, it will conflate actual flakiness with tests that fail consistently and tests that pass consistently. Also, there's the danger that you'll mark it flaky because of the ML flag, but someone else might look at the file and confuse it as a non-flaky test. If this was an acceptable short-term workaround until I can get a more general solution in place that'll help you, the GPU tests, and whatever else we come up with, I'd prefer to do that first (that should happen this week or next). But, if that isn't acceptable, I am open to putting a patch like this in place instead as a temporary workaround until I can fix it properly. Comments on the patch in the next comment.
Comment on attachment 72491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72491&action=review > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:264 > + return '' Can you update test_expectations() to call this routine as well? > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:278 > overrides_path = self.path_from_chromium_base('webkit', 'tools', Can you restructure this to build up a list of overrides files, and then map those onto overrides, e.g.: override_paths = [] if use_drt: override_paths.append( ... path to drt_expectations.txt) if multiple_loads: override_paths.append( ... path to multiple loads) return ''.join([self.test_expectations_from_file(p) for p in override_paths] I think this is slightly clearer. Then, if we end up keeping the idea of multiple test_expectation files, we could -- in a separate patch -- modify the Port interface to just return an ordered list of files for both the main expectations and the overrides files, and do the map/reduce up in the generic layer. > WebKitTools/Scripts/webkitpy/style/checker.py:465 > + basename == 'multiple_loads_expectations.txt'): We should pick a consistent name for expectations files so that we don't have to modify the style checker to have a hard coded list. I suggest we use test_expectations_*.txt, and rename drt_expectations.txt to test_expectations_drt.txt . If this turns out to be hard to do, please modify this code to at least add a FIXME: to do this in a separate patch. On the other hand, if you decide to implement this as part of this patch, please let Tony Chang know, since I think he tracks the drt expectations the most.
Comment on attachment 72491 [details] Patch This patch is old and has pending comments, so R-'ing to remove from the queue.
Is this issue still relevant, or should we close it now?
I'm going to assume we can close this. Feel free to reopen if we still need something.