Closed
Bug 829066
Opened 12 years ago
Closed 12 years ago
tests in settings should test one file at a time
Categories
(Firefox OS Graveyard :: Gaia::Settings, defect)
Tracking
(b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)
RESOLVED
FIXED
People
(Reporter: julienw, Assigned: julienw)
References
Details
Attachments
(1 file, 1 obsolete file)
23.99 KB,
patch
|
etienne
:
review+
vingtetun
:
approval-gaia-v1+
|
Details | Diff | Splinter Review |
In bug 796381, we moved all stuff in the 'device information' page in its own About object Unfortunately the unit tests were not updated to follow this change.
Comment 1•12 years ago
|
||
\o/
Comment 2•12 years ago
|
||
Assigning to Kaze so that we introduce him to the test framework when he has time to fix this.
Assignee: nobody → kaze
Assignee | ||
Comment 3•12 years ago
|
||
WIP
sorry, I didn't see that kaze was made assignee...
We need to wait for bug 816306 to land because he fixed the bug to let it run but it's still not good (as he loads both about.js and settings.js files) and we need to merge this patch and his patch.
Also I changed all |Settings.mozSettings| in about.js to |navigator.mozSettings| which might be a problem; what do you think kaze ?
I'll stop here for now, kaze you can take my patch and finish it if you want :)
Assignee | ||
Comment 4•12 years ago
|
||
Comment on attachment 700498 [details] [diff] [review]
patch v1
BTW, apply this patch with |git am| on a branch.
Assignee | ||
Comment 5•12 years ago
|
||
BTW gaia patch in bug 816306 was just merged.
Assignee | ||
Comment 6•12 years ago
|
||
Current Settings tests loads both about.js and settings.js and this is wrong because unit tests should test only one object at one time. So changing the title to reflect this.
Summary: OMG Kaze broke the tests in settings !!!¡!¡!! → tests in settings should test one file at a time
Assignee | ||
Comment 7•12 years ago
|
||
while I was making a patch to fix tests broken in Bug 840322, I thought that maybe the correct way would be to fix this bug instead.
about_test.js only tests about.js now, removed the call to settings.js and
mocked it instead.
Also made mocks_helper call a function for each step.
---
apps/settings/js/about.js | 2 +-
apps/settings/test/unit/about_test.js | 254 ++++++++++++++++++++
apps/settings/test/unit/mock_navigator_settings.js | 19 +-
apps/settings/test/unit/mock_settings.js | 9 +
apps/settings/test/unit/mocks_helper.js | 47 ++++
apps/settings/test/unit/settings_test.js | 226 -----------------
apps/system/test/unit/mock_navigator_settings.js | 19 +-
apps/system/test/unit/mocks_helper.js | 13 +-
8 files changed, 357 insertions(+), 232 deletions(-)
create mode 100644 apps/settings/test/unit/about_test.js
create mode 100644 apps/settings/test/unit/mock_settings.js
create mode 100644 apps/settings/test/unit/mocks_helper.js
delete mode 100644 apps/settings/test/unit/settings_test.js
Attachment #714427 -
Flags: review?(etienne)
Comment 8•12 years ago
|
||
Comment on attachment 714427 [details] [diff] [review]
patch v2
Review of attachment 714427 [details] [diff] [review]:
-----------------------------------------------------------------
::: apps/system/test/unit/mocks_helper.js
@@ +5,5 @@
>
> MocksHelper.prototype = {
>
> setup: function mh_setup() {
> + this._forEachMock('mSetup');
<3
Attachment #714427 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 9•12 years ago
|
||
master: https://github.com/mozilla-b2g/gaia/commit/3586a01dbae113903de7a772f14ab34ac444e55b
thanks !
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•12 years ago
|
Attachment #700498 -
Attachment is obsolete: true
Assignee | ||
Comment 10•12 years ago
|
||
Comment on attachment 714427 [details] [diff] [review]
patch v2
NOTE: If blocking-basecamp+ is set, just land it for now.
[Approval Request Comment]
Bug caused by (feature/regressing bug #):
User impact if declined:
Testing completed:
Risk to taking this patch (and alternatives if risky): none
The only change to the app file is very small, not risky at all, and really fix the test that was being done at this place.
The other changes are only changes to the unit tests so I think this should be uplifted to prevent future uplifting conflicts.
Attachment #714427 -
Flags: approval-gaia-v1?
Assignee | ||
Updated•12 years ago
|
Attachment #714427 -
Flags: approval-gaia-v1? → approval-gaia-v1?(21)
Updated•12 years ago
|
Assignee: kaze → felash
Comment 11•12 years ago
|
||
Comment on attachment 714427 [details] [diff] [review]
patch v2
a=tests. But make sure that your one liner changes in the Settings app still make sense since this code has changed since then.
Attachment #714427 -
Flags: approval-gaia-v1?(21) → approval-gaia-v1+
Assignee | ||
Comment 12•12 years ago
|
||
The one-liner is in about.js which didn't change at all, so it's ok.
to uplifter: this should land to v1.0.1 too (a=tests)
Comment 13•12 years ago
|
||
v1-train@c481c39548d79dcf948eceda740efb373f2be3e8
Assignee | ||
Comment 14•12 years ago
|
||
v1.0.1: c01954bba4c0592e7dce649be731994de25e555f
You need to log in
before you can comment on or make changes to this bug.
Description
•