948087 - Add log tag to UITestContext.dumpLog messages

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

It should take a Class and the implementation in UITest should append `Class.getName() + ": " + message` to most pre-existing messages (use your judgement).

Comment 1

[Phil]

Attachment: bug948087v1.patch

Hi,

This patch adds a dumpLog method declarations that include a class parameter. I left the old declarations alone because I'm not sure where this method is used and  I didn't want break anything. Also, if you can point me to where this method is being used I can update where this method is called so that the new version of the method is used.

Thanks,
Phil

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8355732 [details] [diff] [review]
bug948087v1.patch

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

Sorry to change the scheme on you, but I thought this over and I think I'd prefer to take a `final String logtag`, rather than the class. Context can also define a `static getLogtag(Class c)` method which `return c.getSimpleName();` (note that I believe this can be done dynamically via stack traces, but I'm not sure the implications). Each class that calls dumpLog should ideally define a `public static final LOGTAG = sContext.getLogtag(Classname)`.

It's more effort to add final logtags to each class but it makes implementation easier for those people writing short tests, e.g. `context.dumpLog(LOGTAG, ...)` rather than `context.dumpLog(getClass(), ...)` or worse in the static method case. Also, it's more flexible if we don't want to use the classname to log. Let me know if you disagree in any way.

You can find the old calls of this method using our MXR tool [1]. An example search would be [2] or [3] (though 3 may be missing some calls). Alternatively, you can use a local tool such as grep or built-in IDE features.

I think it would be better to remove the old declarations so we can enforce good form (i.e. you HAVE to have a log tag - when would you not want to?). If it doesn't break at compile time, it shouldn't break at runtime.

Thanks for your help!

[1]: https://mxr.mozilla.org/
[2]: https://mxr.mozilla.org/mozilla-central/search?string=\.dumpLog&regexp=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
[3]: https://mxr.mozilla.org/mozilla-central/search?string=context\.dumpLog&regexp=1&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

::: mobile/android/base/tests/UITest.java
@@ +158,5 @@
>      @Override
>      public void dumpLog(final String message, final Throwable t) {
>          mAsserter.dumpLog(message, t);
>      }
> +	

nit: We like to remove any excess whitespace (end of line, empty lines, etc.

@@ +159,5 @@
>      public void dumpLog(final String message, final Throwable t) {
>          mAsserter.dumpLog(message, t);
>      }
> +	
> +	@Override

nit: Please use spaces instead of tabs and make sure this is aligned with the method declaration.

@@ +160,5 @@
>          mAsserter.dumpLog(message, t);
>      }
> +	
> +	@Override
> +    public void dumpLog(Class c, final String message) {

nit: `final Class c`. While not strictly necessary, it's good to be consistent.

Comment 3

[Phil]

Attachment: bug948087v2.patch

Hi,

Here is an updated patch with the changes you mentioned. I think you are right about passing in Final String logtag in stead of a Class. It's more consistent with the Android style.

I also set the declaration of LOGTAG as private static final String LOGTAG = ClassName.class.getSimpleName(), instead of public as you suggested, because that follows Android convention as well. 

Thanks,
Phil

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8356643 [details] [diff] [review]
bug948087v2.patch

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

Besides the nit, looks good! Fix that for an r+! ^_^

::: mobile/android/base/tests/components/AboutHomeComponent.java
@@ +22,5 @@
>   * A class representing any interactions that take place on the Awesomescreen.
>   */
>  public class AboutHomeComponent extends BaseComponent {
> +    private static final String LOGTAG = AboutHomeComponent.class.getSimpleName();
> +    

nit: The newline here is good, but there are a few unnecessary spaces here. Can you remove them please?

Comment 5

[Phil]

Attachment: bug948087v3.patch

Removed extra spaces. 

Is there something you are running to pickup these formatting issues that I can run before I submit a patch to save everybody time?

Phil

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8356685 [details] [diff] [review]
bug948087v3.patch

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

Looks good to me! You can add the "checkin-needed" keyword to get this pushed into the tree.

(In reply to Phil from comment #5)
> Is there something you are running to pickup these formatting issues that I
> can run before I submit a patch to save everybody time?

I spot it because it shows up in our diff/review tool. However, my editor, vim, also highlights excess whitespace in red, which helps me get rid of it. Most editors should have similar functionality - perhaps you can Google around for a solution? If you ping me on IRC, I can help find one too.

You could also probably run a sed command to fix it automatically before `qrefresh`ing (and even add it as an hg hook). Something like:

hg status | cut -f2 -d" " | xargs sed -i "s/\s\+$//"

which should get all of the names of all the files you changed and pass it to sed, which removes any end of line whitespace. I didn't test this thoroughly (and there are a lot of edge cases) so I wouldn't recommend using it if any of it looks foreign to you (though feel free to ask for a more specific explanation).

Thanks again for your help!

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/b46d73d9f715

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b46d73d9f715