792031 - Add child role list comparison to tree tests.

Status: RESOLVED WONTFIX

(Core :: Disability Access APIs, defect)

Description

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

Trevor as per chat, is something like this useful? Attachment coming.

Comment 1

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

Attachment: idea1

This is sort of a minimal change and just shows the child lists as we walk the tree. Is this useful or do you want to compare two entire jsonified trees? (or both?)

Comment 2

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

Further improvements might be:

1. running a normalizing pass on the expected tree (shorthand and regular version) into whatever data structure is best.
2. converting the actual tree to the same data structure.
3. doing a deep compare and showing a nice readable result.

(perhaps now or follow up - not sure I can do this soon)

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #2)
> Further improvements might be:
> 
> 1. running a normalizing pass on the expected tree (shorthand and regular
> version) into whatever data structure is best.
> 2. converting the actual tree to the same data structure.
> 3. doing a deep compare and showing a nice readable result.
> 
> (perhaps now or follow up - not sure I can do this soon)

I think that describes the end state I'd like fiarly well.  I'm not sure  how much you'd need to do in part 3, I'd be happy with just saying here is the expected data structure and the actual one they're different.

Comment 4

[Trevor Saunders (:tbsaunde)]

When I look at logs of failures this seems to tell a little more about what's going on so I think its somewhat useful.  However I think David has a plan for doing comment 2 so...

Also I'd say you can remove the existing tests that each individual role is the same and that the length of the array is what is expected since this new test covers both of those.

Comment 5

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

Attachment: patch

Inspired by comment 4, and including some cleanup as per IRC. I'd like to leave the super tree diff to another bug.

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 662211 [details] [diff] [review]
patch

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

f+ for idea, I cannot say the same for implementation ;)

::: accessible/tests/mochitest/common.js
@@ +320,5 @@
>  
>  /**
> + * Test index, sibling, and parent integrity for a given child.
> + */
> +function testTreeSanity(aChild, aIndex, aChildren, aParent)

it's not a public method, it should be under 'private' section

@@ +403,4 @@
>  
> +    // Compare the given and expected child roles as two long strings
> +    var expectedRoles = "";
> +    var actualRoles = "";

I prefer them comma separated

@@ +403,5 @@
>  
> +    // Compare the given and expected child roles as two long strings
> +    var expectedRoles = "";
> +    var actualRoles = "";
> +    var i;

is it 'i' supposed to be used outside of 'for' cycle? if no then use let keyword if you aren't comfortable with 'var' under 'for'

@@ +404,5 @@
> +    // Compare the given and expected child roles as two long strings
> +    var expectedRoles = "";
> +    var actualRoles = "";
> +    var i;
> +    for (i=0; i < accTree.children.length; i++) {

nit: spaces around operator

@@ +416,5 @@
> +      var child = children.queryElementAt(i,nsIAccessible);
> +      actualRoles += "[" + roleToString(child.role) + "]";
> +    }
> +    if (actualRoles || expectedRoles) {
> +      is(actualRoles, expectedRoles, "Do stringified child roles match?");

use isRole function instead of strings comparison

But. You have role comparison when you process an accessible of the tree. If you the idea is that you want to find the last role matched child then I suggest to make testAccessibleTree to return a flag that you use to detect whether you should proceed children further or not

@@ +428,5 @@
>           "Wrong first child of " + prettyName(acc));
>  
>        // nsIAccessible::lastChild
> +      var expectedLastChild = children.length > 0 ?
> +        children.queryElementAt(children.length - 1, nsIAccessible) : null;

why didn't you like childCount thing?

Comment 7

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

Thanks.

(In reply to alexander :surkov from comment #6)

> > +function testTreeSanity(aChild, aIndex, aChildren, aParent)
> 
> it's not a public method, it should be under 'private' section

OK. I had seen some helper functions earlier in the file...

>> > +    var expectedRoles = "";
> > +    var actualRoles = "";
> 
> I prefer them comma separated

OK

> > +    var i;
> 
> is it 'i' supposed to be used outside of 'for' cycle? if no then use let
> keyword if you aren't comfortable with 'var' under 'for'

In js when you use 'var' (as we have been) it is already useable outside the for loop. My thinking was that this makes the scope more explicit, and makes reuse in later for loops more obvious.

> > +    for (i=0; i < accTree.children.length; i++) {
> 
> nit: spaces around operator

OK. Old habits.

> 
> @@ +416,5 @@
> > +      var child = children.queryElementAt(i,nsIAccessible);
> > +      actualRoles += "[" + roleToString(child.role) + "]";
> > +    }
> > +    if (actualRoles || expectedRoles) {
> > +      is(actualRoles, expectedRoles, "Do stringified child roles match?");
> 
> use isRole function instead of strings comparison

I'm not sure I understand. The reason for this bug is to do the string comparison all at once because this helps Trevor debug. (All expected and actual child roles will be shown at once in the result of the 'is' in the test log).

> But. You have role comparison when you process an accessible of the tree.

Where do I process an accessible? (What does this mean?)

> If
> you the idea is that you want to find the last role matched child then I
> suggest to make testAccessibleTree to return a flag that you use to detect
> whether you should proceed children further or not

Not sure I understood this either.

> 
> @@ +428,5 @@
> >           "Wrong first child of " + prettyName(acc));
> >  
> >        // nsIAccessible::lastChild
> > +      var expectedLastChild = children.length > 0 ?
> > +        children.queryElementAt(children.length - 1, nsIAccessible) : null;
> 
> why didn't you like childCount thing?

In reading this code I liked seeing explicitly which children we were counting. In non-test production code I would probably have left it or renamed it more explicitly. What do you think?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #7)
> > > +    var i;
> > 
> > is it 'i' supposed to be used outside of 'for' cycle? if no then use let
> > keyword if you aren't comfortable with 'var' under 'for'
> 
> In js when you use 'var' (as we have been) it is already useable outside the
> for loop. My thinking was that this makes the scope more explicit, and makes
> reuse in later for loops more obvious.

I understand that but nevertheless if you don't need that variable outside cycles then you 'let' inside cycle.

> I'm not sure I understand. The reason for this bug is to do the string
> comparison all at once because this helps Trevor debug. (All expected and
> actual child roles will be shown at once in the result of the 'is' in the
> test log).

Oh, now I see. I think the problem you are trying to solve is the following: when children count is different than expected count then you want to know what exactly was broken. So roles children sets comparison (what you do) provides you more information technically but it seems enough to know what child wasn't matched (that's what I suggested). For that you just walk through children and run testAccessibleTree recursively, if role is matched then you return 'ok', if not then 'not ok' and you don't process next children.

however, if you're trying to solve the problem that the log is too noisy to understand what's going on and to fix that you try to compare all children by roles then it probably makes more sense. But the bug description is not really descriptive to get a good idea what you try to address here. Btw, we don't really need those tree integrity checks in ok results, we can print errors when something goes wrong, it should reduce logging size.

> > why didn't you like childCount thing?
> 
> In reading this code I liked seeing explicitly which children we were
> counting. In non-test production code I would probably have left it or
> renamed it more explicitly. What do you think?

childCount seems shorter, maybe a little bit more readable, up to you.

Comment 9

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

> however, if you're trying to solve the problem that the log is too noisy to
> understand what's going on and to fix that you try to compare all children
> by roles then it probably makes more sense.

Yes exactly :)

> Btw, we
> don't really need those tree integrity checks in ok results, we can print
> errors when something goes wrong, it should reduce logging size.

Sounds good.

Thanks. I'll post another patch.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #9)
> > however, if you're trying to solve the problem that the log is too noisy to
> > understand what's going on and to fix that you try to compare all children
> > by roles then it probably makes more sense.
> 
> Yes exactly :)

the way you do that is sort of unnice since you have to dupe role comparison code

Comment 11

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

(In reply to alexander :surkov from comment #10)
> (In reply to David Bolter [:davidb] from comment #9)
> > > however, if you're trying to solve the problem that the log is too noisy to
> > > understand what's going on and to fix that you try to compare all children
> > > by roles then it probably makes more sense.
> > 
> > Yes exactly :)
> 
> the way you do that is sort of unnice since you have to dupe role comparison
> code

Are you referring to the recursive check in the 'Test accessible properties' area? If so I could pass in a 'skipRootRoleCheck' third parameter when recursing... ?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #11)

> > the way you do that is sort of unnice since you have to dupe role comparison
> > code
> 
> Are you referring to the recursive check in the 'Test accessible properties'
> area? If so I could pass in a 'skipRootRoleCheck' third parameter when
> recursing... ?

is it really necessary to compare roles of all children before looking into children. Would it be enough that report the child (and perhaps its index in the array) that has different role than expected one? It should be easy enough to see what and where is going wrong.

Comment 13

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

(In reply to alexander :surkov from comment #12)
> (In reply to David Bolter [:davidb] from comment #11)
> 
> > > the way you do that is sort of unnice since you have to dupe role comparison
> > > code
> > 
> > Are you referring to the recursive check in the 'Test accessible properties'
> > area? If so I could pass in a 'skipRootRoleCheck' third parameter when
> > recursing... ?
> 
> is it really necessary to compare roles of all children

I think so.

> before looking into
> children. Would it be enough that report the child (and perhaps its index in
> the array) that has different role than expected one?

I think this is not human friendly.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #13)

> > before looking into
> > children. Would it be enough that report the child (and perhaps its index in
> > the array) that has different role than expected one?
> 
> I think this is not human friendly.

so, "section section section entry tree list section group section entry not equal to section section entry entry list section section canvas entry" is human friendly and much more readable I guess than "section is not equal to entry at index 2"?

Comment 15

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

(In reply to alexander :surkov from comment #14)
> (In reply to David Bolter [:davidb] from comment #13)
> 
> > > before looking into
> > > children. Would it be enough that report the child (and perhaps its index in
> > > the array) that has different role than expected one?
> > 
> > I think this is not human friendly.
> 
> so, "section section section entry tree list section group section entry not
> equal to section section entry entry list section section canvas entry" is
> human friendly and much more readable I guess than "section is not equal to
> entry at index 2"?

Are you able to get on IRC?

If you would like to find out what Trevor is asking for I suggest talking to him and then let me know when you guys agree.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #15)

> If you would like to find out what Trevor is asking for I suggest talking to
> him and then let me know when you guys agree.

I think Trevor will join the discussion and comment here. You don't need to be a broken phone :)

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #14)
> (In reply to David Bolter [:davidb] from comment #13)
> 
> > > before looking into
> > > children. Would it be enough that report the child (and perhaps its index in
> > > the array) that has different role than expected one?
> > 
> > I think this is not human friendly.
> 
> so, "section section section entry tree list section group section entry not
> equal to section section entry entry list section section canvas entry" is
> human friendly and much more readable I guess than "section is not equal to
> entry at index 2"?

I say I think yes it is more human friendly.  Note I think what I actually want is something like

{
  role: section
  tag: div
  id: container
  children: {
    role: entry
    tag: input
  }
}

is not
{
  role: section
  tag: div
  id: container
  [
    {
      role: entry
      tag: input
    },
    {
      role: button
      tag: input
      id: button_2
    }
  ]
}

Especially since you only need to print / read all this stuff if the test failed so your trying to debug what's wrong.

Comment 18

[alexander :surkov (:asurkov)]

why wouldn't you like to look at the sources instead? It should be normal practice when error message allows you identify the place where you test fails (but not gives you an answer where the bug is).

Comment 19

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #18)
> why wouldn't you like to look at the sources instead? It should be normal
> practice when error message allows you identify the place where you test
> fails (but not gives you an answer where the bug is).

Well, the test source doesn't tell me what the tree actually looked like does it? ;)

I guess you could not output the expected  tree and just have people look at the source. That woldn't really bother me, but it seems like it might be nice to have it right there side by side like instead of having to open another file and switch between things.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #19)
> (In reply to alexander :surkov from comment #18)
> > why wouldn't you like to look at the sources instead? It should be normal
> > practice when error message allows you identify the place where you test
> > fails (but not gives you an answer where the bug is).
> 
> Well, the test source doesn't tell me what the tree actually looked like
> does it? ;)

I might miss something but you describe the expected tree like 

{ NOTHING: [ // tr
  { NOTHING: [ // th
    { TEXT_LEAF: [ ] }
  ] },
  { NOTHING: [ // td
    { TEXT_LEAF: [ ] }
  ] }
] }

and if you get 'nothing role was expected but we've got entry role at index 0, level 1' perhaps then it should be a good for locating the problem.

> I guess you could not output the expected  tree and just have people look at
> the source. That woldn't really bother me, but it seems like it might be
> nice to have it right there side by side like instead of having to open
> another file and switch between things.

true but it might be not fit into the format of ok/not ok results. anyway the current patch doesn't compare trees it prints two lines of roles and you need to understand what is that.

Comment 21

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

Attachment: patch2

I hope this ends up helping debugging and isn't a waste. This is probably as much time as I'm going to put into this bug.

Comment 22

[Trevor Saunders (:tbsaunde)]

Comment on attachment 662601 [details] [diff] [review]
patch2

>+// Test index, sibling, and parent integrity for a given child. Only report
>+// tests for fail cases (to reduce log noise).
>+function testTreeSanity(aChild, aIndex, aChildren, aParent)

nit, I thought we usually used /* */ comments here?