1017822 - startup-abouthome-dirty test is broken on android

Status: RESOLVED FIXED

(Testing Graveyard :: Eideticker, defect)

Description

[William Lachance (:wlach)]

Just noticed when doing some Android testing that the startup-abouthome-dirty test is broken. We're not preinitializing the profile anymore... not sure exactly how this broke, but we should fix it. Fortunately this is easy to do.

Comment 1

[William Lachance (:wlach)]

Attachment: Fix profile preinitialization

Comment 2

[Bob Clary [:bc] (inactive)]

wlach, I'm really unfamiliar with this code and without a running local instance I feel really handicapped to review this. Upon the face of the patch it looks ok but really it looks like this should be unnecessary. Looking around it appears to me that the issue is in the manifests.

https://github.com/mozilla/eideticker/blob/master/src/tests/fennec/manifest.ini includes abouthome.ini and  abouthomedirty.ini

https://github.com/mozilla/eideticker/blob/master/src/tests/fennec/abouthome.ini defines preInitializeProfile = 0 in the [about:home] section 

https://github.com/mozilla/eideticker/blob/master/src/tests/fennec/abouthomedirty.ini does not define preInitializeProfile but uses the same section [about:home] as abouthome.ini. So in https://github.com/mozilla/eideticker/blob/d8eab6c804a0946e80d36579bdfb76bb8d2f92ad/src/eideticker/eideticker/test.py#L340 when you

self.preinitialize_user_profile = int(
            testinfo.get('preInitializeProfile', 1))

the reason the default value on the get is not returning 1 is because preInitializeProfile was defined as 0 in abouthome.ini's [about:home].

I think abouthomedirty.ini should use a different section name such as [about:homedirty] and that would resolve this without any code changes.

thoughts?

Comment 3

[Bob Clary [:bc] (inactive)]

Comment on attachment 8431083 [details] [diff] [review]
Fix profile preinitialization

Review of attachment 8431083 [details] [diff] [review]:
-----------------------------------------------------------------

r- since I don't think this is necessary and to get it out of my review queue.

Comment 4

[William Lachance (:wlach)]

(In reply to Bob Clary [:bc:] from comment #2)

> I think abouthomedirty.ini should use a different section name such as
> [about:homedirty] and that would resolve this without any code changes.
> 
> thoughts?

Ok, this kind of confused me too. So in fact what you say is not correct -- preInitializeProfile is getting set to 0 because it's 0 in src/tests/manifest.ini (https://github.com/mozilla/eideticker/blob/master/src/tests/manifest.ini#L3). abouthomedirty.ini doesn't inherit from abouthome.ini, so the value there has nothing to do with it.

Pre-initializing the profile is only something we want to do in very special cases (like the abouthomedirty test) as it takes extra time (it starts fennec and stops it again). I would be open to not having multiple ini files in this directory though, as I think it would clean things up and reduce confusion. Let me do up another patch.

Comment 5

[William Lachance (:wlach)]

Attachment: Take 2

Hopefully the logic is easier to understand after this patch.

Comment 6

[Bob Clary [:bc] (inactive)]

Comment on attachment 8432745 [details] [diff] [review]
Take 2

Review of attachment 8432745 [details] [diff] [review]:
-----------------------------------------------------------------

Ok. I was thinking in terms of regular ini files and not manifestdestiny ini files. I'll have to get used to them.

Apart from the logging  and removing the default value on the get, couldn't we have gotten the same result by just adding preInitializeProfile = 1 to abouthomedirty.ini ?

::: src/eideticker/eideticker/test.py
@@ +338,5 @@
>          self.log_checkerboard_stats = log_checkerboard_stats
>          self.profile_file = profile_file
>          self.gecko_profiler_addon_dir = gecko_profiler_addon_dir
> +        self.preinitialize_user_profile = bool(
> +            testinfo.get('preInitializeProfile'))

I guess we should never rely on get with a default value if we have DEFAULT sections in our manifests. Do you know why manifestdestiny doesn't raise an exception if the option is not defined in the manifest? It seems that returning None is prone to hiding errors.

::: src/tests/fennec/manifest.ini
@@ +1,1 @@
>  [include:abouthome.ini]

newline

Comment 7

[William Lachance (:wlach)]

(In reply to Bob Clary [:bc:] from comment #6)
> Comment on attachment 8432745 [details] [diff] [review]
> Take 2
> 
> Review of attachment 8432745 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok. I was thinking in terms of regular ini files and not manifestdestiny ini
> files. I'll have to get used to them.
> 
> Apart from the logging  and removing the default value on the get, couldn't
> we have gotten the same result by just adding preInitializeProfile = 1 to
> abouthomedirty.ini ?

Yes, I think so. Most of this patch is just clean up to try and make sure this error doesn't happen again.

> ::: src/eideticker/eideticker/test.py
> @@ +338,5 @@
> >          self.log_checkerboard_stats = log_checkerboard_stats
> >          self.profile_file = profile_file
> >          self.gecko_profiler_addon_dir = gecko_profiler_addon_dir
> > +        self.preinitialize_user_profile = bool(
> > +            testinfo.get('preInitializeProfile'))
> 
> I guess we should never rely on get with a default value if we have DEFAULT
> sections in our manifests. Do you know why manifestdestiny doesn't raise an
> exception if the option is not defined in the manifest? It seems that
> returning None is prone to hiding errors.

No, I have no idea but I agree this doesn't sound like the best idea. In general I find many of the ways that manifestparser works kinda mysterious. I think at this point :jmaher knows the most about how it works, you might ask him.

Comment 8

[William Lachance (:wlach)]

https://github.com/mozilla/eideticker/commit/d65ff7b36a311bc291e9e22d99b0c16576816622