Somehow my comment got eaten;
-------------------------
Hi hajimehoshi@,

Firstly, I want to apologize, the *actual* problem you are hitting should be detected much earlier rather than this failing all the way at the merge step.

The problem here is that a test is somehow ending up being run on two different shards. This is *not* suppose to happen.

There are a couple of possible causes of this;
 (a) The test is defined somewhere twice.
 (b) The test somehow shares a common name with another tests.

I thought both these issues should be detected these days. IIRC jeffcarp@ or qyearsley@ was working this?

In all these cases when this happens you will effectively get random behaviour when run locally as the test will end up running twice and will silently overwrite the output of the first run.

I think https://bugs.chromium.org/p/chromium/issues/detail?id=730250 is the bug?
-------------------------

Hi tansell@, thank you for taking a look. What is the current status?

> I think https://bugs.chromium.org/p/chromium/issues/detail?id=730250 is the bug?

Maybe, but the problem is found after that bug was fixed.

Thanks,

ping tansell@,

I'm afraid this is a blocker of crbug/757374

I added the same fix in https://chromium-review.googlesource.com/c/chromium/src/+/647050 as https://chromium-review.googlesource.com/c/chromium/src/+/461722/47/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/merge_results.py#286, which was reversed.

Apologies for the delay, just getting back from being OOO. I haven't dug into this very far but here's a guess:

In cssom/ there are two files that share the same name but have different extensions: MediaList.html and MediaList.xhtml.[1]

In TestResultWriter[2], the output files are stripped of their extensions. This means both of the above files would be written to the same filename: cssom/MediaList-stderr.txt.

To confirm this we could try renaming MediaList.xhtml to MediaList2.xhtml in your CL and seeing if the same error occurs.

[1] https://github.com/w3c/web-platform-tests/tree/master/cssom
[2] https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py?l=134

Thank you for responding. So, I was wondering if you think this is still a problem and are going to fix this.

Hi hajimehoshi,

Please see jeffcarp's comment. You CL *does* actually contain a bug as he identified. 

The build system *should* give you a better error message when you do something like this, but it *is* something that we should be stopping you from submitting.

Thanks!

Tim 'mithro' Ansell

Ah OK, I'll try renaming the file MediaList.xhtml to MediaList2.xhtml in the next time. Thank you for elaborating.

Hm, WPTManifest.generate_manifest in the presubmit fails with renaming the filename without any messages...

Instead of renaming, how about including the extension in TestResultWriter?

Fixing WPT_BASE_MANIFEST.json solved the presubmit error, but I am not sure that is allowed.

https://chromium-review.googlesource.com/c/chromium/src/+/697405

As discussed at the CL (#12), it seems not feasible to fix Chromium tools and output files because a lot of tools and output files depend on the fact that there is not duplicated basename files.

Instead, I'm trying to fix filename of wpt upstream https://github.com/w3c/web-platform-tests/pull/7569

Thanks Hajime!

Suspicious tests that have duplicated base names:

fast/dom/createElement-with-column
external/wpt/dom/nodes/ParentNode-querySelector-All-content
external/wpt/dom/nodes/Document-createElement-namespace-tests/bare_svg
external/wpt/dom/nodes/Document-createElement-namespace-tests/bare_mathml
external/wpt/dom/nodes/Document-createElement-namespace-tests/minimal_html
external/wpt/dom/nodes/Document-createElement-namespace-tests/xhtml_ns_changed
external/wpt/dom/nodes/Document-createElement-namespace-tests/svg
external/wpt/dom/nodes/Document-createElement-namespace-tests/empty
external/wpt/dom/nodes/Document-createElement-namespace-tests/xhtml_ns_removed
external/wpt/dom/nodes/Document-createElement-namespace-tests/xhtml
external/wpt/dom/nodes/Document-createElement-namespace-tests/bare_xhtml
external/wpt/dom/nodes/Document-createElement-namespace-tests/mathml
external/wpt/dom/nodes/Document-contentType/resources/blob
external/wpt/common/dummy
external/wpt/encoding/resources/utf-32-big-endian-bom
external/wpt/encoding/resources/utf-32-big-endian-nobom
external/wpt/encoding/resources/utf-32-little-endian-nobom
external/wpt/encoding/resources/utf-32-little-endian-bom
external/wpt/resource-timing/resources/resource_timing_test0

At least, createElement-with-column is now problematic in https://chromium-review.googlesource.com/c/chromium/src/+/722301. Not sure the others.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3b3e16e84077fec37dd1381b18bad5be154d14d2

commit 3b3e16e84077fec37dd1381b18bad5be154d14d2
Author: Hajime Hoshi <hajimehoshi@chromium.org>
Date: Wed Oct 18 19:13:15 2017

Rename createElement-with-column.xml -> createElement-with-column2.xml

Now there are files that have same basenames (
createElement-with-column.html and createElement-with-column.xml), and
this makes some Chromium test tools confused (See the build log at
https://chromium-review.googlesource.com/c/chromium/src/+/722301).

This CL avoids this problem by just renaming the file as workaround. We
should fix some tools to detect such duplication in the future anyway.

Bug: 764662
Change-Id: I0493bb43d074527950558f81104776706e6df427
Reviewed-on: https://chromium-review.googlesource.com/725580
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509830}
[add] https://crrev.com/3b3e16e84077fec37dd1381b18bad5be154d14d2/third_party/WebKit/LayoutTests/fast/dom/createElement-with-column2-expected.txt
[rename] https://crrev.com/3b3e16e84077fec37dd1381b18bad5be154d14d2/third_party/WebKit/LayoutTests/fast/dom/createElement-with-column2.xml

It'd be easier to enforce the uniqueness of basenames (as a lint). As for WPT, there is already an issue on the topic: https://github.com/w3c/web-platform-tests/issues/7570, but the lint being discussed is WPT-specific, shall we perhaps add a lint to cover all LayoutTests?

I think a lint to cover all layout tests would be reasonable, possibly in addition to checking in run-webkit-tests after finding the list of tests. What would be the simplest way to do it, maybe a presubmit check?

Removing this from the trooper queue as it doesn't appear to be a trooper issue. (Feel free to re-add it and correct me if I'm wrong about that.)

This is a P2 but nobody is assigned to work on it. robertma@, if we had a lint for this, what do you think we should do when an automatic wpt import fails because of colliding file names? It seems a bit annoying to have to rename files on either side to avoid this.

Would it be feasible to just handle duplicate basenames instead? Is it merge_results.py that would have to be updated?

As far as I understand, a change to merge_results.py would not be sufficient; run-webkit-tests assumes that no two layout tests have the same path minus extension, since the baseline file path is basename + baseline extension, i.e. two tests named foo.html and foo.htm would have the same baseline path, which would lead to conflicts.

Philip, I think we already reached a conclusion from the previous discussion: fixing the underlying work is unknown amount of substantial work in our infra (and also WebKit's), so enforcing unique basenames in a directory is the reasonable compromise.

We *should* add a lint in the wpt upstream first. This will prevent colliding filenames altogether so we don't need to worry about import failures. The lint will then be synced to Chromium as well.

We *may* add another lint in Chromium to enforce the same rule over the whole LayoutTests, as the lint above would only cover LayoutTests/external/wpt after being synced to Chromium.

I'm assigning the issue to myself. The work will happen in the upstream though.

robertma@, any update on this, should it be a P2?

Assign to kyleju@ as the upstream issue was assigned to him.