950821 - Minor bug in presetPanel logic: wrong comparison to 'undefined'

Status: RESOLVED FIXED

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

Description

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Currently[1] it looks like we are incorrectly checking for null values while initializing spans with data-name fields (we check if result[key] == 'undefined', where I think the intent was to check if typeof result[key] was undefined).

I'm just getting started with gaia dev, so this seemed like a nice starting bug. I'll open a pull request presently.

One thing I'm not sure of, where's the best place to add a unit test for this little bug? We'd need to have a mock mozSettings response with some empty values corresponding to a span with data-name attr. Still digging into the unit tests for this app, I'd appreciate guidance.

[1] https://github.com/mozilla-b2g/gaia/blob/d7a6ea/apps/settings/js/settings.js#L488

Comment 1

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: link to pull request

Comment 2

[Fabien Cazenave [:kaze]]

(In reply to Jared Hirsch [:_6a68] from comment #0)
> Currently[1] it looks like we are incorrectly checking for null values while
> initializing spans with data-name fields (we check if result[key] ==
> 'undefined', where I think the intent was to check if typeof result[key] was
> undefined).

This was a temporary workaround for an old issue… and now we’re afraid to remove it. Arthur, WDYT? Can we get rid of those?

(In reply to Jared Hirsch [:_6a68] from comment #0)
> I'm just getting started with gaia dev

Thanks for reminding us the dirt we left under the carpet. :-)

Comment 3

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

No prob! Thanks for the quick feedback.

It seems like there's no clean way to actually test these flows--we'd need to mock out the response object sent back by the mozSettings.get('*'), and then verify that it really did or really didn't prefill a test panel correctly. I haven't read closely through the test code yet, but it seems like this might not be all the way there? Pointers very welcome to relevant spots in the source.

Comment 4

[Fabien Cazenave [:kaze]]

Sorry for the noise Arthur, I just realize that this comparison to 'undefined' has been introduced to remove a very specific bug that was related to the MAC address (which is stored in a mozSetting). See bug 880617. Shame on me.

I think we can safely remove this comparison from the master branch now. Mihai, WDYT?

Comment 5

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Fabien, great find! It didn't occur to me that actually checking for the string 'undefined' might be a workaround for a bug elsewhere in the system :-P

Comment 6

[Fabien Cazenave [:kaze]]

Not our most glorious moment. :-/
We thought of adding a proper comment in connectivity.js but we forgot for this file.

Comment 7

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to Fabien Cazenave [:kaze] from comment #4)
> Sorry for the noise Arthur, I just realize that this comparison to
> 'undefined' has been introduced to remove a very specific bug that was
> related to the MAC address (which is stored in a mozSetting). See bug
> 880617. Shame on me.
> 
> I think we can safely remove this comparison from the master branch now.
> Mihai, WDYT?

Indeed that was the reason why the apparently redundant check was added. I don't think at this point there is any other setting that might store the string 'undefined' => it should be safe to remove it.

Thanks for asking about this!

Comment 8

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Thanks for the input, guys!

Hmm. So, fixing the 'undefined' check is easy. Adding tests seems a little more ambitious: although some core app features are exercised by some of the unit tests, I don't see a dedicated file that just tests the settings framework logic (panel twiddling and settings cache behavior).

I can just timebox it and take a swing? Or, if someone has a start on this stashed in a branch somewhere, let me know and I can work from that. I'm a bit concerned I'll burn a lot of time just trying to get the settings app into a test harness--not that this isn't useful experience, but I do have higher-priority stuff to do later this week, and I'd like to get this done or just close as meh/wontfix.

A third option, I guess, would be to fix the check without adding tests. I feel funny about that.

Thoughts? :-)

Comment 9

[Fabien Cazenave [:kaze]]

(In reply to Jared Hirsch [:_6a68] from comment #8)
> I don't see a dedicated file that just tests the settings
> framework logic (panel twiddling and settings cache behavior).

I’ve just filed bug 952472.

> A third option, I guess, would be to fix the check without adding tests.

I’d prefer you to add a comment to this test explaining why it’s there, with a reference to bug 880617. I think we can survive with an untested patch as long as it only contains comments. :)

Sorry to turn this “good first bug” into a comment fix, but this clarification will still be appreciated.

Comment 10

[Fabien Cazenave [:kaze]]

Comment on attachment 8348231 [details] [review]
link to pull request

Clearing the review flag, please r?-me again when you’re ready.

Comment 11

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: updated comment-only pull request

Comment 12

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(In reply to Fabien Cazenave [:kaze] from comment #9)
> I’d prefer you to add a comment to this test explaining why it’s there, with
> a reference to bug 880617. I think we can survive with an untested patch as
> long as it only contains comments. :)
> 
> Sorry to turn this “good first bug” into a comment fix, but this
> clarification will still be appreciated.

No worries! This totally makes sense, thanks for your help. I've opened a pull request for the comment fix.

Comment 13

[Fabien Cazenave [:kaze]]

Merged in master: https://github.com/mozilla-b2g/gaia/commit/b8b6173472d86068483a1a9623d14bb71dbf02df