If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

startup-abouthome-dirty test is broken on android

RESOLVED FIXED

Status

Testing Graveyard
Eideticker
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: wlach, Assigned: wlach)

Tracking

Trunk
x86_64
Linux

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 8431083 [details] [diff] [review]
Fix profile preinitialization
Attachment #8431083 - Flags: review?(bclary)

Comment 2

3 years ago
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

3 years ago
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.
Attachment #8431083 - Flags: review?(bclary) → review-
(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.
Created attachment 8432745 [details] [diff] [review]
Take 2

Hopefully the logic is easier to understand after this patch.
Attachment #8431083 - Attachment is obsolete: true
Attachment #8432745 - Flags: review?(bclary)

Comment 6

3 years ago
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
Attachment #8432745 - Flags: review?(bclary) → review+
(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.
https://github.com/mozilla/eideticker/commit/d65ff7b36a311bc291e9e22d99b0c16576816622
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

2 months ago
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.