1057996 - [Messages][Tests] ThreadUI unit tests aren't run if full

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Oleg Zasypkin [:azasypkin]]

Typo in patch for bug 1053968 caused exception on test suite parsing stage, so that the rest of tests weren't executed. Currently test agent just swallows such exceptions that makes it really hard to identify :(

Comment 1

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL

Hey Steve,

Here is the patch that fixes typo and a few tests that were failing when typo was fixed.

Thanks!

Comment 2

[Oleg Zasypkin [:azasypkin]]

We still need to make something with test agent\mocha to avoid that in the future, will check if something similar is filed against it

Comment 3

[Steve Chung [:steveck]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #2)
> We still need to make something with test agent\mocha to avoid that in the
> future, will check if something similar is filed against it

Just some quick question before running the tests: Sorry that I could not find out the typo at first glance... why do we need to replace the assertion with verifying the attribute in this patch? Although verifying the attribute is fine for me, it still makes sense that verifying the L10n setAttributes call correctly.

Comment 4

[Oleg Zasypkin [:azasypkin]]

The typo is here: https://github.com/mozilla-b2g/gaia/pull/23267/files#diff-d3f17b4905e6f17ad136aabbdc262db9L2823

Assert on the same level as "test" function and it throws on test parsing stage because "button" isn't available yet :)

Comment 5

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #3)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #2)
> Although verifying the attribute
> is fine for me, it still makes sense that verifying the L10n setAttributes
> call correctly.
Yeah, it's fine for me too, but I believe I changed it only for the cases where "setAttributes" wasn't called at all (download(-ing) button) and tests were invalid in fact, but they weren't executed due to exception thrown on "parsing" stage.

Comment 6

[Steve Chung [:steveck]]

Comment on attachment 8478201 [details] [review]
GitHub pull request URL

Thanks for the clear explanation!
Just some small nit, so r=me

Comment 7

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #6)
> Comment on attachment 8478201 [details] [review]
> GitHub pull request URL
> 
> Thanks for the clear explanation!
> Just some small nit, so r=me

Thanks for review! Nits fixed and landed.

Master: https://github.com/mozilla-b2g/gaia/commit/8b315117fe19b20bf8c3253a796aa1b5be14b47f

Comment 8

[Julien Wajsberg [:julienw]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #2)
> We still need to make something with test agent\mocha to avoid that in the
> future, will check if something similar is filed against it

Bug 1036955 :)

Comment 9

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (PTO 08/19 -> 09/15; contact schung for SMS matters) from comment #8)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #2)
> > We still need to make something with test agent\mocha to avoid that in the
> > future, will check if something similar is filed against it
> 
> Bug 1036955 :)

Nice, nice, nice! :)