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)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
Tracking Status
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: julienw, Assigned: julienw)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
\o/
Assigning to Kaze so that we introduce him to the test framework when he has time to fix this.
Assignee: nobody → kaze
Attached patch patch v1 (obsolete) — Splinter Review
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 :)
Comment on attachment 700498 [details] [diff] [review] patch v1 BTW, apply this patch with |git am| on a branch.
BTW gaia patch in bug 816306 was just merged.
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
Attached patch patch v2Splinter Review
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)
Blocks: 841815
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+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #700498 - Attachment is obsolete: true
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?
Attachment #714427 - Flags: approval-gaia-v1? → approval-gaia-v1?(21)
Assignee: kaze → felash
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+
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)
v1-train@c481c39548d79dcf948eceda740efb373f2be3e8
v1.0.1: c01954bba4c0592e7dce649be731994de25e555f
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: