1236490 - Run the tv system app in the system domain

Status: RESOLVED WONTFIX

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

Description

[Alberto Pastor [:albertopq]]

As a first step for merging both apps into 1, we want to move all the smart-system files into the system folder at build time, and picking the right files using a different html.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:merge-tv > mozilla-b2g:master

Comment 2

[Alberto Pastor [:albertopq]]

Comment on attachment 8703576 [details] [review]
[gaia] albertopq:merge-tv > mozilla-b2g:master

Hey Luke! I cleaned the patch up a little bit. Now we copy html and locales at build time as well (in order to have always the last changes from the smart-system folder).

The only thing I miss here is a simple test that verifies that the TV boots up correctly in the system domain.

What do you think?

Comment 3

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8703576 [details] [review]
[gaia] albertopq:merge-tv > mozilla-b2g:master

Hi Alberto,

It looks good to me overall. However, I'm afraid I don't have enough time to verify it in detail since I'm still stuck in TV 2.5 blockers. I would suggest we hear Rex's feedback as well.

Comment 4

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8703576 [details] [review]
[gaia] albertopq:merge-tv > mozilla-b2g:master

Looks ok to me.
I've tested it with WIP patch of re-enabling some TV integrating test from Fischer, and it works ok my local. But I'm not sure if it also works on treeherder (Hopefully yes given it still runs smart-system integration tests). We can prove it once Fischer lands his patch.

Comment 5

[Alberto Pastor [:albertopq]]

Comment on attachment 8703576 [details] [review]
[gaia] albertopq:merge-tv > mozilla-b2g:master

TV tests running correctly in the system domain \o/

https://public-artifacts.taskcluster.net/RVqmCy26Ti-ygKd6fv7QlQ/0/public/logs/live_backing.log

Rex, final review? Thanks!

Comment 6

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8703576 [details] [review]
[gaia] albertopq:merge-tv > mozilla-b2g:master

Sorry for the late review. It looks good to me.

I guess we have some tv integration test that need to move domain from smart-system to system too, but they can be done once we re-enable the tests.

Comment 7

[Alberto Pastor [:albertopq]]

:rexboy, all the integration tests are already running in the system domain since this PR. Thanks for the review!

master: https://github.com/mozilla-b2g/gaia/commit/67aab9371d1a58ca2d068f9d30bb0ba2669db04f

Comment 8

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:revert-1236490 > mozilla-b2g:master

Comment 9

[Alberto Pastor [:albertopq]]

Found an issue when logging in the browser

Comment 10

[Alberto Pastor [:albertopq]]

Revert: https://github.com/mozilla-b2g/gaia/commit/384fa14ed02ba7ac6491f4ae7b744a06ed8bef87

Comment 11

[Alberto Pastor [:albertopq]]

Mhm, it seems that the issue happens in master already. Creating an integration test to find when was regressed -> bug 1248376

Comment 12

[Alberto Pastor [:albertopq]]

Ok, so the issue finally seems caused by this bug, but I still need to check what's the root cause. I don't think is going to be a big thing given the nature of this bug (a domain name, or a folder not being moved). Now we have integration tests to cover this in bug 1248376

Comment 13

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:merge-tv > mozilla-b2g:master

Comment 14

[Alberto Pastor [:albertopq]]

Could you please run a smoke test with this patch? Thanks!

Comment 15

[Mike Lien[:mlien]]

Hi Cindy, please arrange resource to perform smoketest and the simulator build with Alberto's patch is under: https://drive.google.com/open?id=0B9Zi9TqbRWsddkZrbXl3VmlzX0k

After test finished, please post the results here for reference to see if any regression issue

Comment 16

[Mike Lien[:mlien]]

Hi Alberto, the patch will make production build cannot bootup successfully

I guess the reason is due to inconsistent code change between apps-engineering.list and apps-production.list
the production build will still get system app from tv_apps fodler instead of apps folder

Comment 17

[Alberto Pastor [:albertopq]]

Sorry, misspelled tv_apps instead of apps in apps-production.list. Should be fixed now. Thanks!

Comment 18

[cindylee]

Attachment: Smoke_Test_For_Bug 1236490.xlsx

About the smoke test result,please refer to the attachment.
Column C is the result of daily smoke test and column D is the result of this patch.
I compared the both results and here is the conclusion:
We only found some existing issues and didn't find any regression issue on this patch.

Thanks.

Comment 19

[Mike Lien[:mlien]]

(In reply to cindylee from comment #18)
> Created attachment 8720683 [details]
> Smoke_Test_For_Bug 1236490.xlsx
> 
> About the smoke test result,please refer to the attachment.
> Column C is the result of daily smoke test and column D is the result of
> this patch.
> I compared the both results and here is the conclusion:
> We only found some existing issues and didn't find any regression issue on
> this patch.
> 
> Thanks.

Hi Alberto, refer to this attachment, it seems doesn't break basic functions. I think we can land it to move forward.

Another thing, from the previous problem: the inconsistent code between engineering build and production build, do you think we need to test both builds or we have a solution to make sure the differences between two apps list/builds.

Comment 20

[Alberto Pastor [:albertopq]]

That sounds great!

Regarding the inconsistency between production and engineering, I don't think we need to run both builds every time. This patch is a very special one, which changes the way the smart-system app gets built. So, in my opinion, this is the only patch which would make sense to test both environments (checking the list of apps should be enough). In following patches, just testing 1 environment should be enough.

Thanks!

Comment 21

[Alberto Pastor [:albertopq]]

Comment on attachment 8719744 [details] [review]
[gaia] albertopq:merge-tv > mozilla-b2g:master

Just changed 1 line [1], but let's get a final review here to ensure that I'm not missing anything.

Thanks!

[1] https://github.com/mozilla-b2g/gaia/pull/34064#discussion-diff-53000363R134

Comment 22

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8719744 [details] [review]
[gaia] albertopq:merge-tv > mozilla-b2g:master

Looks good! Thanks for helping on the FxA issue.

Comment 23

[Alberto Pastor [:albertopq]]

Thanks Luke!

master: https://github.com/mozilla-b2g/gaia/commit/d7de878f60003244cf91046cafe06c1b6cd6ecc9

Comment 24

[GitHub Autolander Bot]

Attachment: [gaia] mozilla-b2g:revert-34064-merge-tv > mozilla-b2g:master

Comment 25

[Luke Chang [:lchang] (inactive)]

reverted: https://github.com/mozilla-b2g/gaia/commit/9fa9c3f943997d5b0578090c8ce3c7bbde0d7673


Since the system merging plan has been suspended, we might need to revert some changes in order not to interfere with the transition plan and TV 2.6.