Closed
Bug 1341390
Opened 4 years ago
Closed 2 years ago
Ensure dom/tests/js and dom/tests/html are covered by automated tests and then delete them
Categories
(Core :: DOM: Core & HTML, defect, P5)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: jmaher, Assigned: dpino)
References
Details
(Keywords: good-first-bug)
Attachments
(1 file)
48.73 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•4 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)
Comment 2•4 years ago
|
||
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)
![]() |
||
Comment 3•4 years ago
|
||
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)
Updated•4 years ago
|
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•2 years 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)
![]() |
||
Comment 5•2 years ago
|
||
> 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 6•2 years ago
|
||
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+
![]() |
||
Updated•2 years ago
|
Assignee: nobody → dpino
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•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/357559d3ec0d
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•2 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•