1420683 - TEST-UNEXPECTED-FAIL | comm/chat/modules/test/test_filtering.js on TaskCluster

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Jorg K (CEST = GMT+2)]

Attachment: chattest.patch

TEST-UNEXPECTED-FAIL | comm/chat/modules/test/test_filtering.js | test_permissiveMode - [test_permissiveMode : 222] "<span style=\\"font-family: normal; font-style: normal; font-weight: normal; font-size: 15px;\\">foo</span>" == "<span style=\\"font-style: normal; font-weight: normal; font-size: 15px; font-family: normal;\\">foo</span>"

Patch by Aceman:

Try (I hope I got this right):
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=839f0972bb14e1abdb3f98504bf709faff781e80

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: chattest.patch

(Now with Unix EOL)

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 8931920 [details] [diff] [review]
chattest.patch

Nice job and very elegant. I hope the tests pass now.

Comment 3

[Jorg K (CEST = GMT+2)]

Worked perfectly, I'll get this landed with a slightly improved commit message.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e3cec7382989
fix chat test test_filtering.js by sorting style attributes to get a reliable result. r=jorgk

Comment 5

[:aceman]

Comment on attachment 8931920 [details] [diff] [review]
chattest.patch

This has a perf hit also for normal chat operation (not just tests) so I wanted a proper chat peer to check this. There may also be other ways to implement this.
This didn't need to be rushed into the tree.

Comment 6

[Jorg K (CEST = GMT+2)]

Sorry. As far as I understand, chat happens in "user time", so no one will notice a 0.1ms delay. Maybe it will also make restoring prior conversations slower. That said, there's only one call here:
https://dxr.mozilla.org/comm-central/rev/207bc898d11bac95d901dcf3e998d54203f85c73/chat/content/convbrowser.xml#553
and that function already has heaps of processing, so I don't think the extra cycles will be a "performance hit".

Comment 7

[Jorg K (CEST = GMT+2)]

One could add a parameter to cleanupImMarkup() to determine whether the sorting is needed.

Comment 8

[:aceman]

Yes, that could be a solution. Let a chat peer decide what is better for chat. Maybe the sorting of attributes could be beneficial also for core code, as for sanitizing the css a reliable order could be useful.

Comment 9

[:aceman]

For the record, this is not a spurious problem on server Taskcluster, I could see the failure (different css attributes order) locally on Linux.

Comment 10

[Patrick Cloke [:clokep]]

Comment on attachment 8931920 [details] [diff] [review]
chattest.patch

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

I'm probably not the best to ask about this code, hopefully Florian has an opinion!

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8931920 [details] [diff] [review]
chattest.patch

I don't like that this is adding code that only matters to tests in a hot path of code displaying messages in the product, but I also doubt this would have a significant impact on performance unless we see a profile showing it. And I would really like us to profile this code and improve it's performance in the future.

Comment 12

[Jorg K (CEST = GMT+2)]

Where does this comment leave us? Back it out and have the test fail? Add another parameter as per comment #7? Other ideas? "I don't like it" sadly doesn't offer a better approach.

Comment 13

[Florian Quèze [:florian]]

(In reply to Jorg K (GMT+1) from comment #12)
> Where does this comment leave us? Back it out and have the test fail?

No. Just leave things as they have been for the last 11 days, and hopefully some time in a not so distant future we'll get a better picture of what matter and what doesn't about the performance of this code.

I'm hoping to find time to put a profiler at this but it's unlikely to happen in the next two weeks.