Avoid panics when using HTMLAnchorElement attribute setters

[broesamle]

expected: OK still explicitly mentioned in .ini file. Shall I remove it?

Fixes #10877.

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/script/dom/htmlanchorelement.rs

[KiChjang]

Yes, remove this. By default, tests are expected to PASS (or OK), so if there's no .ini file, the test framework will expect the test file to pass.

[KiChjang]

Speaking of which, why isn't this a WPT test?

[Ms2ger]

Please put this in the directory of upstream tests, add assert_*s that the setter works correctly, and also test all the other setters you changed.

[broesamle]

@jdm points to an overlap regarding the tests with #10876 and suggests:

Test: We should create a test that exercises each of the getters and setters individually in tests/wpt/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/.

Shall I just move it there?

[broesamle]

to rephrase my question (making me hesitate to just move the test):
Is there a specific location where to put it, before it formally enters the WPT repo?

what you called:

the directory of upstream tests

[Ms2ger]

tests/wpt/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/ is perfect.

[broesamle]

well, I tried to merge the the upstream(servo/servo) master into a local branch of my fork.

The conflict in MANIFEST.json looks a bit awkward to resolve manually :-/
"Rebuilding" MANIFEST.json could be easier:
./mach test-wpt --manifest-update
Would that work, does that make sense?

[jdm]

It would, but I recommend ./mach test-wpt --manifest-update SKIP_TESTS to avoid running any tests.

[bors-servo]

☔ The latest upstream changes (presumably #10946) made this pull request unmergeable. Please resolve the merge conflicts.

[broesamle]

done the rebase...at least the conflict is gone :-)

[broesamle]

Manually merged MANIFEST.json
all tests that I touched can be run (so the MANIFEST should be ok), despite some did not pass.

[broesamle]

The command "bash etc/ci/manifest_changed.sh" exited with 1.
:-(

so, what is the best practice of resolving merge conflicts from MANIFEST.json?

[jdm]

Huh, it should be enough to undo any changes to MANIFEST.json and run ./mach test-wpt --manifest-update SKIP_TESTS.

[broesamle]

let's see ... this time I used the automatic update again.

[broesamle]

you were right, @jdm, I should not have merged MANIFEST.json by hand.

[jdm]

Looking good! I think we can make the new test easier to read and modify, however.
-S-awaiting-review +S-needs-code-changes

---

Reviewed 1 of 2 files at r2, 1 of 3 files at r4, 2 of 4 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

tests/wpt/web-platform-tests/html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_attribute-getter-setter.html, line 11 [r6] (raw file):
There's a lot of duplication in each of these tests. I think we can do something like this:

function test_setter(property, old, new) {
  /* initialize a to known url */
  assert_equals(a[property], old);
  a[property] = new;
  assert_equals(a[property], new);
}

var tests = [["hash", "#somehash", "#someother"], ...];
for (var i = 0; i < tests.length; i++) {
  test(function() {
    test_setter(tests[i][0], tests[i][1], tests[i][2]);
  }, "Getter and setter for attribute of anchor element: " + tests[i][0])
}

---

Comments from Reviewable

[broesamle]

agreed. I rewrote the test according to your suggestion -- with the slight 'complexication' that I have an additional check for the old and new url.

Which already produced an unexpected (to me) fail in the search setter

assert_equals: expected "http://google.com?hi" but got "http://google.com/?hi"

Is it valid, if the setter adds the / after the hostname, here?
(From my quick look in the specs I did not come to a conclusion.)

otherwise all tests pass nicely.

[highfive]

New code was committed to pull request.

[broesamle]

did it. (forget my former concern) ... its late here in berlin.

[jdm]

@bors-servo: r+
Thank you very much!

[bors-servo]

📌 Commit 751733a has been approved by jdm

[broesamle]

Thank you for your patience! I learned a lot about servo on the way :-)

[bors-servo]

⌛ Testing commit 751733a with merge fe83feb...

[bors-servo]

💔 Test failed - windows

[mbrubeck]

@bors-servo retry

• network error

[bors-servo]

⌛ Testing commit 751733a with merge 545da38...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows