Ensure dom/tests/js and dom/tests/html are covered by automated tests and then delete them

RESOLVED FIXED in Firefox 64

Status

()

defect
P5
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: jmaher, Assigned: dpino)

Tracking

({good-first-bug})

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

I cannot find reference in-tree to js/ or html/ being used or mentioned.  Can we get a readme file to indicate why these exist, or just delete these files?
Reporter

Comment 1

2 years ago
:overholt, can you help determine what these directories/files are used for?  hg history shows the cvs import from 2007 as all changes (other than wide sweeping mass changes)
Flags: needinfo?(overholt)
Reporter

Updated

2 years ago
Blocks: 1339232
Reporter

Updated

2 years ago
Depends on: 1341356
Reporter

Updated

2 years ago
No longer depends on: 1341356
I see no references, either. Comments like Boris' in bug 345521 comment 8 seem to back this up. Let's confirm with Boris.
Flags: needinfo?(overholt) → needinfo?(bzbarsky)
These are manual tests, afaict.

We could try ensuring that they're ported to one of our automated suites when possible, then delete them.
Flags: needinfo?(bzbarsky)
Keywords: good-first-bug
Priority: -- → P5
Summary: dom/tests/js and dom/tests/html do not appear to be used at all → Ensure dom/tests/js and dom/tests/html are covered by automated tests and then delete them
Assignee

Comment 4

9 months ago
Summary:

All the tests in `dom/tests/html` and `dom/tests/js` are covered, at least, by web-platform-tests. Both directories can be removed.

The tests check very basic DOM functionality and several Javascript events.

TLDR:

* dom/tests/html/jshandlers.html:

The test doesn't longer work. It breaks returning the following error:

TypeError: l is null; can't access its "onmousedown" property[Learn More]

What the test tries to do is to verify that `onmousedown`, `onmouseup` and `onmousemove` events of an anchor tag work. To do so, it overwrites these events and prints out a message using `dump` [1] (now deprecated).

Covered by: html/browsers/the-window-object/window-properties.https.html and ./html/editing/dnd/events/events-suite-manual.html.

* dom/tests/js/attributes.html:

The tests verifies the following operations related with DOM attributes:

`
- attribute.getNamedItem == getAttributeNode: true
- attribute BGCOLOR=#ffffff
- changing attribute node value changes attribute value: true
- return value of removeNamedItem is attribute node: true
- removing attribute changes attribute count: true
- changing disembodied attribute value works: true
- removing attribute node removes attribute: true
`

Covered by: `dom/nodes/attributes.html`.

The test relies on `dump`.

* dom/tests/js/class.html:

Tests DOM node className field.

Covered by: `dom/nodes/Node-properties.html`.

* dom/tests/js/clone.html:

Tests DOM node `cloneNode()` method.

Covered by: `dom/nodes/Node-cloneNode.html`.

* dom/tests/js/docfrag.html:

Tests several DOM operations: `createDocumentFragment()`, `replaceChild()`, `createTextNode()`, `appendChild()`

Covered by: `dom/nodes/Node-replaceChild.html`.

* dom/tests/js/id.html:

Tests `getElementById()`.

Covered by: `dom/nodes/Document-getElementById.html`.

* dom/tests/js/lists.html:

Iterates over document.images, document.links and document.anchors printing out some of the nodes attribute values via `getDOMAttribute()`.

The test returns the following error:

```
document.images[x].getDOMAttribute is not a function[Learn More]
```

`getDOMAttribute` is no longer part of the DOM API. Apparently it does the same as `node.getAttribute(name)`.

The test relies on `dump`.

Covered by: `dom/nodes/attributes.html`.

* dom/tests/js/style1.html:

Defines several CSS styles and perform several style changes in some document tags via `style` attribute. These changes
are done by calling a function (changeColor) every 40 ms. The function doesn't fully work printing out the following
error in the console:

```
TypeError: h is undefined; can't access its "style" property[Learn More]
style1.html:103:3
TypeError: can't access property "style" of undefined[Learn More]
...
```

Covered by: `css/cssom/cssstyledeclaration-csstext.html` and `css/css-transitions/events-007.html`.

* dom/tests/js/style2.html:

Similar to `style1.html`.

The test prints out the following error in the console:

```
TypeError: can't access property "style" of undefined[Learn More] style1.html:103:3
TypeError: document.documentElement.childNodes[1].childNodes[1] is undefined; can't access its "childNodes" property[Learn More]
```

Covered by: `css/cssom-view/scrolling-quirks-vs-nonquirks.html`.

* dom/tests/js/write2.html:

Tests `document.write()` and `document.close()`.

Covered by: `./html/webappapis/dynamic-markup-insertion/document-write/033.html` and `./html/webappapis/dynamic-markup-insertion/document-write/50.html`.

* dom/tests/js/write.html:

Tests `document.writeln()`.

Covered by: `./html/webappapis/dynamic-markup-insertion/document-write/033.html`.

There are other JavaScript tests in dom/html:

- DumpHTML.js: iterates an HTML document and prints out in console.
- DumpTree.js: Traverse a tree of DOM nodes.
- HTMLString.js: Starting from document.documentElement, prints out tree. 
- simple.js: Access several DOM node and attribute fields such as: node.hasChildNodes, node.tagName, node.nextSibling, attribute.name or attribute.value.
- ssheets.js: Iterates over a stylesheet and access several stylesheet properties.
- timer.js: Uses setTimeout and setInterval.

I didn't exhaustively looked into web-platform-tests for these JavaScript tests, but likely all this is covered since they are very basic operations.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Window/dump
Attachment #9017120 - Flags: review?(bzbarsky)
> Covered by: `dom/nodes/Node-properties.html`.

What dom/tests/js/class.html is really testing is that the class change triggers restyling and relayout.  The  dom/nodes/Node-properties.html wpt doesn't test that, but I'm pretty sure other wpts do.

> Tests `getElementById()`.

No, tests that changing the id does restyling and relayout.  Again probably covered by wpts, but not by `dom/nodes/Document-getElementById.html
Comment on attachment 9017120 [details] [diff] [review]
Remove-unused-tests-dom-tests-html-and-dom-tests-js.patch

r=me.   Thank you for the patch and the analysis!  It's very much appreciated.
Attachment #9017120 - Flags: review?(bzbarsky) → review+
Assignee: nobody → dpino

Comment 7

8 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/357559d3ec0d
Remove unused tests dom/tests/html and dom/tests/js.  r=bzbarsky

Comment 8

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/357559d3ec0d
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.