1064600 - [Gallery] preliminary modularization before full gallery refresh

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

Before we start serious work on the Gallery refresh in bug 1024258, we need to do some preliminary modularization of the existing code base. This will not be a full-blown refactoring, and will mainly focus on moving existing code into modules in separate files, to increasing the readability and testability of the code.

Comment 1

[David Flanagan [:djf]]

Attachment: work-in-progress pull request

Comment 2

[Justin D'Arcangelo [:justindarc]]

Re-assigning to myself to resume work on this first-pass refactor to free up some of djf's bandwidth.

Comment 3

[Justin D'Arcangelo [:justindarc]]

Attachment: WIP pull-request (master)

Rebased djf's original PR

Comment 4

[Justin D'Arcangelo [:justindarc]]

Attachment: WIP pull-request (master)

Comment 5

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8506373 [details] [review]
WIP pull-request (master)

David: Please review the last commit of this PR. The first 5 commits are carried over from your original PR and the last commit is mine where I added unit tests for the 4 new modules. I also made a minor tweak in pick.js to change `onclick=` to `addEventListener('click')` to add consistency and improve testability.

Diego: Have a look over this entire PR and provide feedback on the approach.

If I may interject my opinion here, I feel like while this approach very loosely defines "modules", it should hopefully be good enough to at least get us started down a path of improving the architecture of Gallery. That being said, it does lack some of the benefits of AMD modules such as inter-module dependency management. In several of the modules here, such as spinner.js, this is a complete non-issue. But, in a more complex module such as pick.js, it becomes much more obvious. The pick.js module depends on 10 globals to be defined outside the scope of the module and our only mechanism of declaring this is in a /* global */ comment. Testing pick.js was a bit cumbersome at first because of all these external dependencies and I feel like this would have been a little more straight-forward in an AMD-based approach. However, at the end of the day, having these loosely-defined modules is still 100x better than a single, monolithic gallery.js file. Going forward with this approach, we should try to keep inter-module dependencies to a minimum whenever we can.</opinion>

Comment 6

[Diego Marcos [:dmarcos]]

Comment on attachment 8506373 [details] [review]
WIP pull-request (master)

This is definitively a step in the right direction! We went from 13 to 48 tests and the architecture becomes more visible making the app easier to understand. I agree with Justin that introducing an established module pattern would make dependency management/loading easier and our code more reusable. We don't have to introduce such a change on this patch but let's keep it in mind for follow up work.

Comment 7

[David Flanagan [:djf]]

Comment on attachment 8506373 [details] [review]
WIP pull-request (master)

Justin,

I've left a few minor comments on github. But the main reason I'm setting r- is that I'd like to understand why you're using a completely mock DOM instead of using loadBodyHTML() to load the actual index.html file and test the code in its actual context.  Right now the way you've structured the tests constrains developers to always use $() to query elements or tests will break. That seems pretty brittle.

I haven't written tests using loadBodyHTML() myself, but it looks like there are hundreds of Gaia test files that do use it, so I suspect that this is the established way to go.

Comment 8

[Diego Marcos [:dmarcos]]

(In reply to David Flanagan [:djf] from comment #7)
> Comment on attachment 8506373 [details] [review]
> WIP pull-request (master)
> 
> Justin,
> 
> I've left a few minor comments on github. But the main reason I'm setting r-
> is that I'd like to understand why you're using a completely mock DOM
> instead of using loadBodyHTML() to load the actual index.html file and test
> the code in its actual context.  Right now the way you've structured the
> tests constrains developers to always use $() to query elements or tests
> will break. That seems pretty brittle.
> 
> I haven't written tests using loadBodyHTML() myself, but it looks like there
> are hundreds of Gaia test files that do use it, so I suspect that this is
> the established way to go.

In gallery we have a monolithic HTML that makes difficult to test components in isolation. If you look at the gaia apps that use loadBodyHTML (bluetooth or communications for instance) they have the DOM broken down in small testable chunks (transfer.html, onpair.html, message.html in the case of bluetooth). Justin was trying to come up with a solution to avoid loading the whole app DOM which goes against the purpose of unit testing. I've always been reluctant to touch any DOM while unit testing. DOM is UI and it should be cover by integration tests instead. Mocking the DOM might not be such a bad compromise in this case since we have a monolithic HTML and still don't have integration tests in place.

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8506373 [details] [review]
WIP pull-request (master)

After discussion with Justin, I'm converting my review to an r+ with the provision that we file a followup bug to come back and make these tests less brittle.

Comment 10

[David Flanagan [:djf]]

(In reply to Diego Marcos [:dmarcos] from comment #8).
> 
> In gallery we have a monolithic HTML that makes difficult to test components
> in isolation. 

Agreed. And my argument is that for modules that depend on the markup in index.html, it is fine to include that markup in the test and have the test depend on index.html as well. We don't get pure isolation, but at least we get an easy to write test without the brittleness the comes from an incompletely mocked DOM.

> If you look at the gaia apps that use loadBodyHTML (bluetooth
> or communications for instance) they have the DOM broken down in small
> testable chunks (transfer.html, onpair.html, message.html in the case of
> bluetooth). 

But there are also plenty of apps that load their entire index.html file as well.

> Justin was trying to come up with a solution to avoid loading
> the whole app DOM which goes against the purpose of unit testing. 

Understood. Pragmatically, however, I fear that the brittleness of that approach will cause problems, and I'd rather sacrifice some purity and define our "unit" as the module plus its index.html context.

I've
> always been reluctant to touch any DOM while unit testing. DOM is UI and it
> should be cover by integration tests instead. Mocking the DOM might not be
> such a bad compromise in this case since we have a monolithic HTML and still
> don't have integration tests in place.

I'd rather keep it even simpler and just use the DOM. The DOM is not an independent unit that we need to write tests for. If we trust the DOM, then I don't see any real problem with writing unit tests that rely on the DOM when we're testing modules that depend on index.html.

Comment 11

[Justin D'Arcangelo [:justindarc]]

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/bfafc5a2ca03480c0c33abaacd35ee0d0ade3b8e