638313 - Improve accessible/events/test_docload.html

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

Spun off: https://bugzilla.mozilla.org/show_bug.cgi?id=637644#c23

Requires at least investigation. Filing to keep on our radar.

Comment 1

[alexander :surkov (:asurkov)]

I don't understand what is suggestion.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

These tests were getting some discussion in #developers. We should ask people to add constructive criticism here. I haven't dug in at all.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> These tests were getting some discussion in #developers. We should ask
> people to add constructive criticism here. I haven't dug in at all.

After two months there's no description. Is there any chance to get it?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Sure.  Apply the nsDocShell change from https://bug637644.bugzilla.mozilla.org/attachment.cgi?id=516319 then run this test and tell me why it fails.  Is it _supposed_ to fail in that situation, or is just making assumptions about implementation details that it shouldn't be making assumptions about?  How is anyone outside a11y supposed to tell what this test is testing and how are we supposed to make core changes that might cause this test to go orange due to it having undocumented dependencies and there being no one to ask for an explanation of why it's going orange?

Comment 5

[alexander :surkov (:asurkov)]

The mochitest path says about what it tests, so events/test_docload.html means document loading events are tested for document accessible. Then you should know how a11y works in general and know about a11y mochitest helper functions, otherwise it may be not easy to understand what it tests and what fails. 

So in your case:

failed | Doubled event { event type: reorder, target: [' no node info ', role: app root, name: 'Nightly', address: 0x50bd788]} in test with ID = 'open dialog 'about:''.

means there's unexpected reorder event fired on application accessible when you open "about:" page in new window. When new window is open then accessible document is created, it's appended to application accessible as a child and reorder event is fired. So since we get two reorder events then it means, another document was created or destroyed and appended or removed from application accessible children.

Then accessible tree is tested and it's fine (two child documents for application accessible) what means document accessible changes are more crazy than extra creation or removement.

and finally, there's no document accessible for about:document.

I think only by debugging it's possible to answer what's going on here. I assume your patch makes layout to call into a11y more what gets crazy the a11y about documents.

Getting back to this bug, honestly I still don't have ideas how to improve a11y tests to make them more understandable.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Closing as part of a batch cleanup of old untouched bugs. This bug hasn't been touched for at least 2000 days.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Sure, this bug is just a problem for everyone else working on other parts of the codebase...

Comment 8

[Alexandre Poirot [:ochameau]]

I'm getting hit by this test on bug 1405342.
I think less abstraction and better error messages would help.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9864e9af332dae79b027cb87d8e1d6fccd47b484&selectedJob=136520968
https://treeherder.mozilla.org/logviewer.html#?job_id=136520968&repo=try
TEST-PASS | accessible/tests/mochitest/events/test_docload.html | The document accessible for '../events/docload_wnd.html' is not in cache! 
TEST-PASS | accessible/tests/mochitest/events/test_docload.html | The document accessible for iframe is in cache still! 
Buffered messages finished
TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/events/test_docload.html | Scenario #0 of test with ID = 'open dialog '../events/docload_wnd.html'' failed. Dupe reorder event. 
eventQueue_processNextInvoker@chrome://mochitests/content/a11y/accessible/tests/mochitest/events.js:365:17
eventQueue_processNextInvokerInTimeout/<@chrome://mochitests/content/a11y/accessible/tests/mochitest/events.js:441:43

Ideally, we should have an assertion or a stack that would say exactly where the failure comes from.
I imagine it is around this openDialogWnd, but I'm not sure...
http://searchfox.org/mozilla-central/source/accessible/tests/mochitest/events/test_docload.html#151-213

Comment 9

[Alexandre Poirot [:ochameau]]

David, I would appreciate some help here.

I can't proceed with bug 1405342 because of this failing tests.
You can apply this patch to reproduce:
 $ wget https://reviewboard.mozilla.org/r/188014/diff/raw/ -O patch
 $ patch -p1 < patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4c711a41d308f3d7b403794f1c2640547941595&filter-searchStr=a11y
https://treeherder.mozilla.org/logviewer.html#?job_id=138943347&repo=try&lineNumber=3251
INFO - TEST-PASS | accessible/tests/mochitest/events/test_docload.html | Wrong previous sibling of ['document node', address: [object HTMLDocument], role: chrome window, name: 'Accessible events testing for document', address: 0x7fe4d5c27e50] 
INFO - TEST-PASS | accessible/tests/mochitest/events/test_docload.html | Wrong value of property 'role' for ['document node', address: [object HTMLDocument], role: chrome window, name: 'Accessible events testing for document', address: 0x7fe4d5c27e50]. 
INFO - TEST-PASS | accessible/tests/mochitest/events/test_docload.html | The document accessible for '../events/docload_wnd.html' is not in cache! 
INFO - TEST-PASS | accessible/tests/mochitest/events/test_docload.html | The document accessible for iframe is in cache still! 
INFO - Buffered messages finished
INFO - TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/events/test_docload.html | Scenario #0 of test with ID = 'open dialog '../events/docload_wnd.html'' failed. Dupe reorder event. 

The abstraction of this test is really hard to follow.
I think the test has a race that only appears when you make subtle changes to docshell.
It only fails with debug build and not with optimized.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Over to Yura for triage.

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 638313 patch

This patch splits docload test into individual ones and fixes possible races due to timeouts and execute soons. I believe it also addresses concerns in comment 9

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 8921927 [details] [diff] [review]
638313 patch

Review of attachment 8921927 [details] [diff] [review]:
-----------------------------------------------------------------

it's very tempting to move all these tests into docload subfolder as events folder gets larger. looks nice, r=me

::: accessible/tests/mochitest/events/test_docload_busy.html
@@ +31,5 @@
> +          type: EVENT_STATE_CHANGE,
> +          get target() {
> +            return getAccessible("iframe").firstChild;
> +          },
> +          check(aEvent) {

it it busy state change event? If it is, then I like this test variant more than the existing one, but could you please make sure we handle busy state change event here?

@@ +82,5 @@
> +  <pre id="test">
> +  </pre>
> +
> +  <div id="testContainer"><iframe id="iframe" src="about:" style="visibility: hidden;"></iframe></div>
> +  <div id="eventdump"></div>

nit: eventdump is no longer needed

::: accessible/tests/mochitest/events/test_docload_embedded.html
@@ +51,5 @@
> +    // //////////////////////////////////////////////////////////////////////////
> +    // Do tests
> +
> +    // Debug stuff.
> +    // gA11yEventDumpID = "eventdump";

nit: no eventdump pls

@@ +85,5 @@
> +  <pre id="test">
> +  </pre>
> +
> +  <div id="testContainer"><iframe id="iframe"></iframe></div>
> +  <div id="eventdump"></div>

nit: and here

::: accessible/tests/mochitest/events/test_docload_root.html
@@ +89,5 @@
> +      this.getID = () => `open dialog '${aURL}'`;
> +    }
> +
> +    function closeDialogWnd() {
> +      this.eventSeq = [ new invokerChecker(EVENT_FOCUS, getAccessible(document)) ];

good

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c641a06bd8a7
splitting and making test_docload tests more reliable. r=surkov

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c641a06bd8a7

Comment 15

[Alexandre Poirot [:ochameau]]

Thanks, it improved the results!

But there is still some intermittent, this time it appears on QuantumRender builds, opt and debug:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=97136fef3ac445764f2d7d4e29f51dd1ada1b269&selectedJob=140171465

This one is intermittent on QuantumRender opt builds:
  FAIL | accessible/tests/mochitest/events/docload/test_docload_shutdown.html | Scenario #0 of test with ID = 'open dialog '../../events/docload/docload_wnd.html'' failed. Dupe reorder event. 
  https://treeherder.mozilla.org/logviewer.html#?job_id=140171465&repo=try

But this one look permanent on QuantumRender debug builds:
  TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/events/test_focus_tabbox.xul | assertion count 1 is more than expected 0 assertions
  https://treeherder.mozilla.org/logviewer.html#?job_id=140170641&repo=try