1150051 - C-C TB: EXCEPTION: formatted size is not numeric: 'Read'

Status: RESOLVED FIXED

(Thunderbird :: Testing Infrastructure, defect)

Description

[ISHIKAWA, Chiaki]

Hi,

Starting a few days ago (March 29th, JST), I began seeing the following error during |make mozimill| test suite runs locally (I am running locally created full debug build C-C TB)

SUMMARY-PASS | test-message-size.js::setupModule
SUMMARY-UNEXPECTED-FAIL | test-message-size.js | test-message-size.js::test_byte_message_size
  EXCEPTION: formatted size is not numeric: 'Read'
    at: test-message-size.js line 51
       _help_test_message_size test-message-size.js:51 1
       test_byte_message_size test-message-size.js:57 3
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestDirectory frame.js:525 7
       runTestDirectory frame.js:707 3
       Bridge.prototype._execFunction server.js:179 10
       Bridge.prototype.execFunction server.js:183 16
       Session.prototype.receive server.js:283 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-UNEXPECTED-FAIL | test-message-size.js | test-message-size.js::test_kb_message_size
  EXCEPTION: formatted size is not numeric: 'Read'
    at: test-message-size.js line 51
       _help_test_message_size test-message-size.js:51 1
       test_kb_message_size test-message-size.js:61 3
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestDirectory frame.js:525 7
       runTestDirectory frame.js:707 3
       Bridge.prototype._execFunction server.js:179 10
       Bridge.prototype.execFunction server.js:183 16
       Session.prototype.receive server.js:283 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-UNEXPECTED-FAIL | test-message-size.js | test-message-size.js::test_mb_message_size
  EXCEPTION: formatted size is not numeric: 'Read'
    at: test-message-size.js line 51
       _help_test_message_size test-message-size.js:51 1
       test_mb_message_size test-message-size.js:65 3
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestDirectory frame.js:525 7
       runTestDirectory frame.js:707 3
       Bridge.prototype._execFunction server.js:179 10
       Bridge.prototype.execFunction server.js:183 16
       Session.prototype.receive server.js:283 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-PASS | test-message-size.js::teardownModule

Here is the code in question.
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-display/test-message-size.js#51

36 function _help_test_message_size(index, unit) {
37   be_in_folder(folder);
38 
39   // Select the nth message
40   let curMessage = select_click_row(index);
41   // Look at the size column's data
42   let tree = mc.folderDisplay.tree;
43   let sizeCol = tree.columns[11];
44   let sizeStr = tree.view.getCellText(index, sizeCol);
45 
46   // Note: this assumes that the numeric part of the size string is first
47   let realSize = curMessage.messageSize;
48   let abbrSize = parseFloat(sizeStr);
49 
50   if (isNaN(abbrSize))
51     throw new Error("formatted size is not numeric: '"+sizeStr+"'");
52   if (Math.abs(realSize/Math.pow(1024, unit) - abbrSize) > 0.5)
53     throw new Error("size mismatch: '"+realSize+"' and '"+sizeStr+"'");
54 }


I have a suspiction that sizeStr does not contain proper numerical value, and maybe looking at incorrect cell (?) [wrong column position, etc.]
Or it may be that somehow the drawing has not finished by the time
this routine is called (i.e., tree is not in good shape, etc.)

I have not seen this error before (in the last few years) and so
it is either a new test(?) or something broke last week.

TIA

Comment 1

[ISHIKAWA, Chiaki]

Aha, I dumped the value when the isNaN( abbrSize ) is true.
It turns out tree.columns[] is not a simple number itself.
Anway, it seems that the position where sizeStr should be
fetched is not at 11th position, but 12th position.

abbrSize was isNaN
sizeCol=[object TreeColumn]
tree.columns[10]=[object TreeColumn]
tree.columns[12]=[object TreeColumn]
i=0:tree.view.getCellText(index,tree.columns[i])=
i=1:tree.view.getCellText(index,tree.columns[i])=
i=2:tree.view.getCellText(index,tree.columns[i])=
i=3:tree.view.getCellText(index,tree.columns[i])=Huge Shindig Yesterday
i=4:tree.view.getCellText(index,tree.columns[i])=
i=5:tree.view.getCellText(index,tree.columns[i])=Emily Ekberg
i=6:tree.view.getCellText(index,tree.columns[i])=Felix Flowers
i=7:tree.view.getCellText(index,tree.columns[i])=Emily Ekberg
i=8:tree.view.getCellText(index,tree.columns[i])=
i=9:tree.view.getCellText(index,tree.columns[i])=02/01/2000 02:00
i=10:tree.view.getCellText(index,tree.columns[i])=02/01/2000 02:00
i=11:tree.view.getCellText(index,tree.columns[i])=Read
i=12:tree.view.getCellText(index,tree.columns[i])=1.0 MB
i=13:tree.view.getCellText(index,tree.columns[i])=
i=14:tree.view.getCellText(index,tree.columns[i])=Local Folders

I have no idea why there is this shift by one.
Either the test is mistaken or
something within TB changed.

TIA

Comment 2

[[e-mail]]

(In reply to ISHIKAWA, Chiaki)
> Here is the code in question.
> 43   let sizeCol = tree.columns[11];
Bah, whose idea was it to retrieve named items by number? This should use tree.columns.sizeCol instead.

(In reply to ISHIKAWA, Chiaki from comment #1)
> I have no idea why there is this shift by one.
Because the tree now has an extra column...

Comment 3

[ISHIKAWA, Chiaki]

(In reply to [e-mail] from comment #2)
> (In reply to ISHIKAWA, Chiaki)
> > Here is the code in question.
> > 43   let sizeCol = tree.columns[11];
> Bah, whose idea was it to retrieve named items by number? This should use
> tree.columns.sizeCol instead.

I thought "11" was suspicious.

> 
> (In reply to ISHIKAWA, Chiaki from comment #1)
> > I have no idea why there is this shift by one.
> Because the tree now has an extra column...

Yeah I have figured it out by now finally :-)
I was afraid that there may not be a good fix if the positions shifted.
I did not know the code well, so I am relieved to learn
retrieving the value by property/attribute name was possible.

BTW, Bug 36489 is interesting. 
I would have used "To:" instead of icon, but that can wait.
I am opposed to new features until low-level bugs are exterminated, but
the feature proposed in Bug 36489 is useful, indeed. We can use the
crumpled notebook screen's horizontal width effectively.
(And it was first proposed in 2000!? I wish TB had this back in 2003.)
Thank you for working on that bug!

Comment 4

[ISHIKAWA, Chiaki]

Attachment: Fix by using the property name to access sizeCol

Here is a patch. I use the property name sizeCol to access the value, 
and this removes the errors during local |make mozmill| run.

TIA

Comment 5

[:aceman]

Comment on attachment 8588241 [details] [diff] [review]
Fix by using the property name to access sizeCol

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

I haven't tried this yet. There also seems to be a .getNamedColumn() method at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/NsITreeColumns .

::: mail/test/mozmill/folder-display/test-message-size.js
@@ +40,5 @@
>    // Select the nth message
>    let curMessage = select_click_row(index);
>    // Look at the size column's data
>    let tree = mc.folderDisplay.tree;
> +  let sizeCol = tree.columns.sizeCol

Missing ; at the end.

Comment 6

[ISHIKAWA, Chiaki]

Thanks to Joshua Cranmer's fix, mozmill test is running again on try server.

And I see the same errors in the submitted jobs there

For example, the job's mozmill busted log shows the same errors.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=dbc9e7bc1bcc

(In reply to :aceman from comment #5)
> Comment on attachment 8588241 [details] [diff] [review]
> Fix by using the property name to access sizeCol
> 
> Review of attachment 8588241 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I haven't tried this yet. There also seems to be a .getNamedColumn() method
> at
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/NsITreeColumns .

I will look into it.
For some reason, correspondentCol does now show up where it is supposed to be.

> 
> ::: mail/test/mozmill/folder-display/test-message-size.js
> @@ +40,5 @@
> >    // Select the nth message
> >    let curMessage = select_click_row(index);
> >    // Look at the size column's data
> >    let tree = mc.folderDisplay.tree;
> > +  let sizeCol = tree.columns.sizeCol
> 
> Missing ; at the end.

Thank you for catching this subtle issue.

It now gets interesting.
Believe it or not, I did not get any syntax errors when I tried to run
the modified test locally. I think I need to look into carefully why
JS interpreter did not complain OR
why mozmill framework failed to pick up the syntax error.
(Or can it be that ";" just before "let" keyword can be omitted in JS?!)

Anyway, I will create an updated patch.

Comment 7

[:aceman]

(In reply to ISHIKAWA, Chiaki from comment #6)
> > Missing ; at the end.
> 
> Thank you for catching this subtle issue.
> 
> It now gets interesting.
> Believe it or not, I did not get any syntax errors when I tried to run
> the modified test locally. I think I need to look into carefully why
> JS interpreter did not complain OR
> why mozmill framework failed to pick up the syntax error.
> (Or can it be that ";" just before "let" keyword can be omitted in JS?!)

I seem to remember JS automatically guesses you wanted to have ; there so it was not a syntax error.
But due to this feature sometimes it can guess wrong :)

Comment 8

[ISHIKAWA, Chiaki]

Attachment: Fix by using the property name to access sizeCol (take 2, with semicolon)

Here is an updated patch with semicolon.

Yes, it seems that the semicolon can be optional if newline follows immediately.

TIA

Comment 9

[[e-mail]]

Comment on attachment 8589753 [details] [diff] [review]
Fix by using the property name to access sizeCol (take 2, with semicolon)

Looks good to me.

Automatic semicolon insertion happens in a number of places. The worst case I found was this:
> return 1/*
> */+1;
The above returns 2. But the below returns undefined, because a semicolon is automatically inserted.
> return /*1
> */+1;

Comment 10

[ISHIKAWA, Chiaki]

(In reply to [e-mail] from comment #9)
> Comment on attachment 8589753 [details] [diff] [review]
> Fix by using the property name to access sizeCol (take 2, with semicolon)
> 
> Looks good to me.

Thank you for the review. (Yes, it has passed |make mozmill| localy, of course).
I will put checkin-needed keyword.

> 
> Automatic semicolon insertion happens in a number of places. The worst case
> I found was this:
> > return 1/*
> > */+1;
> The above returns 2. But the below returns undefined, because a semicolon is
> automatically inserted.
> > return /*1
> > */+1;

This is insane! No wonder there is a lint-like tool for JS, too.

Comment 11

[ISHIKAWA, Chiaki]

I think the patch needs to go into C-C.

Comment 12

[:aceman]

Yes, but the tree was closed all the time. But it is on a good track.
https://treeherder.mozilla.org/#/jobs?repo=comm-central

Jcranmer, can you open the tree? It seems all platforms build now.

Comment 13

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

https://hg.mozilla.org/comm-central/rev/56afbcedf65e

Comment 14

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #12)
> Yes, but the tree was closed all the time.

Right.

> But it is on a good track.
> https://treeherder.mozilla.org/#/jobs?repo=comm-central
> 
> Jcranmer, can you open the tree? It seems all platforms build now.

Thank you, Jcranmer, for all the hard work!