866011 - Create unit tests for FMRadio

Status: RESOLVED FIXED

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

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Any update on this bug?

Comment 2

[Mark Shiao]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #1)
> Any update on this bug?

Yes, I'm working on it and have the test for the frequency dialer in place.  Going to continue with adding test for other portions of the app.

Comment 3

[Mark Shiao]

Attachment: Link to pull request

Hi Pin,

I've written some unit tests for FM at the request of Tim. Can you please review and let me know your thoughts?

Thanks,
Mark

Comment 4

[Pin Zhang [:pzhang] (inactive)]

(In reply to Mark Shiao from comment #3)
> Created attachment 747811 [details]
> Link to pull request
> 
> Hi Pin,
> 
> I've written some unit tests for FM at the request of Tim. Can you please
> review and let me know your thoughts?
> 
> Thanks,
> Mark

Cool~
I will dive into the details, and feedback you later.

Comment 5

[Pin Zhang [:pzhang] (inactive)]

(In reply to Pin Zhang [:pzhang] from comment #4)
> (In reply to Mark Shiao from comment #3)
> > Created attachment 747811 [details]
> > Link to pull request
> > 
> > Hi Pin,
> > 
> > I've written some unit tests for FM at the request of Tim. Can you please
> > review and let me know your thoughts?
> > 
> > Thanks,
> > Mark
> 
> Cool~
> I will dive into the details, and feedback you later.

Havn't go through the whole patch, sorry for one quick question about this:
  mozFMRadio = {
    frequencyLowerBound: 87.5,
    frequencyUpperBound: 108
  };

I faked a mozFMRadio object in fm/js/fm.js, so I can write the UI on desktop, does the above codes override the faked object? If yes, how about the other interfaces of mozFMRadio? and why don't we just use the mozFMRadio object defined in fm/js/fm.js?

Comment 6

[Pin Zhang [:pzhang] (inactive)]

(In reply to Pin Zhang [:pzhang] from comment #4)
> (In reply to Mark Shiao from comment #3)
> > Created attachment 747811 [details]
> > Link to pull request
> > 
> > Hi Pin,
> > 
> > I've written some unit tests for FM at the request of Tim. Can you please
> > review and let me know your thoughts?
> > 
> > Thanks,
> > Mark
> 
> Cool~
> I will dive into the details, and feedback you later.

@Mark, I have commented your pr, please check it. 

I have one question about test-suite naming, do we have any convention? Like this:
  suite('#favorite list', function() { 

Intuitively, #favorite should be a CSS-selector, i.e. a node which is named `favorite`, but we only defined `fav-list` in the index.html.

Comment 7

[Mark Shiao]

(In reply to Pin Zhang [:pzhang] from comment #6)
> (In reply to Pin Zhang [:pzhang] from comment #4)
> > (In reply to Mark Shiao from comment #3)
> > > Created attachment 747811 [details]
> > > Link to pull request
> > > 
> > > Hi Pin,
> > > 
> > > I've written some unit tests for FM at the request of Tim. Can you please
> > > review and let me know your thoughts?
> > > 
> > > Thanks,
> > > Mark
> > 
> > Cool~
> > I will dive into the details, and feedback you later.
> 
> @Mark, I have commented your pr, please check it. 
> 
> I have one question about test-suite naming, do we have any convention? Like
> this:
>   suite('#favorite list', function() { 
> 
> Intuitively, #favorite should be a CSS-selector, i.e. a node which is named
> `favorite`, but we only defined `fav-list` in the index.html.

Hi Pin,

I've updated with your changes.  Please have a look and let me know what you think.

Thanks,
Mark

Comment 8

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 747811 [details]
Link to pull request

> 
> Hi Pin,
> 
> I've updated with your changes.  Please have a look and let me know what you
> think.
> 
> Thanks,
> Mark

Hi, @Mark, it looks good to me, I have commented about the nits, please fix them, and then r=me.

BTW, I think you can squash all the commits to keep the history more cleaner, :)

Comment 9

[Mark Shiao]

Attachment: Link to pull request

Hi Pin,

Updated and ready for final review

Thanks,
Mark

Comment 10

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 749672 [details]
Link to pull request

(In reply to Mark Shiao from comment #9)
> Created attachment 749672 [details]
> Link to pull request
> 
> Hi Pin,
> 
> Updated and ready for final review
> 
> Thanks,
> Mark

Hi Mark, there is still one formatting nit you missed I think, the other parts look good enough to me, r=me

It would be just a nit fixing, and I think you don't have to ask for another review, :), and thanks for your nice work.

Comment 11

[Mark Shiao]

landed on master

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Thank you both!

Comment 13

[Julien Wajsberg [:julienw]]

Hey,

I _think_ these tests are not working, see https://travis-ci.org/mozilla-b2g/gaia/builds/7221960