880643 - B2G RIL: Add a code quality test on try server for RIL javascript code in gecko

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: marionette test case for checking ril code quality

Benefit of running static code analysis
=======================================
Javascript is a dynamic language; thus even the syntax error could not be caught during the build time. It might cause power-on fail when errors occur in RIL part. By using the tool, you could find the errors first before build/falsh and also improve the code quality.


Tool
====
JSHint: a community-driven tool and also contributed by some mozillians.

The tool need some customization so that the check could be suitable for RIL.
Refer to https://taiwan.etherpad.mozilla.org/216


Bind it with our develop flow
=============================
local: add jshint check as pre-commit hook for your vcs (ex: git, mercurial)
       => guarantee the quality of code commit by you.
global: add a try server test to run the check
       => check everyone's commit, guarantee the code quality in repository.
       => mission of this bug!!


My attempt
==========
Made a marionette test for code quality check by jshint (as attachment)
* read the content of jshint script (written in js)
* read the content of file we want to check (ex: ril_worker.js)
* convert the above content into javascript string and pass them to marionette

It works well when I run it on local pc by

<b2g home> $ ./test.sh marionette

... but fail on try server


Current issue on try server
===========================
We need to read the content of file we want to check.
But it's hard to locate it on the server.

Try server will make the test package first and then run it.
The test file will be here:

/builds/slave/test/build/tests/marionette/tests/dom/system/gonk/tests/marionette/test_ril_code_quality.py

Where could we find the file 'ril_worker.js'? Is it possible?

I even try to add a mechanism in test case to search the file on try server. But it still cannot found it in 20mins (timeout of each case); thus it cause the fail.

If we know how to locate the source file on try server. We could complete this test case on try.

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

Hi Jonathan,

Is it possible to locate the src file when running the marionette test on try server?
For example:

Here is the test case I created in the source tree.
dom/system/gonk/tests/marionette/test_ril_code_quality.py

I would like to read the content of ril_worker.js when running this case.
We could find the file in the following path. 

dom/system/gonk/ril_worker.js                  ... source
obj-gecko/dist/bin/modules/ril_worker.js       ... after build

On the try server, the test case is located here.
/builds/slave/test/build/tests/marionette/tests/dom/system/gonk/tests/marionette/test_ril_code_quality.py

So, could we find the ril_worker.js on try server?
If it is possible, what is the path.

Thank you.

Comment 2

[Jonathan Griffin (:jgriffin)]

From a test running in the build, this file is available at the url "resource://gre/modules/ril_worker.js"; is that sufficient?

Comment 3

[Szu-Yu Chen [:aknow] (inactive)]

Unfortunately, I can't let it work.
What I want it to read the file contents as a pure string in marionette python environmet.
Components.utils.import("resource://gre/modules/ril_worker.js", SCOPE) will evaluate the script content into an object SCOPE. All the comments or format in original file are missing.

Comment 4

[Jonathan Griffin (:jgriffin)]

Can you use XMLHttpRequest or an XPCOM API to read that file?

Relying on the relative path to the file on the test slave would mean that the test would fail locally, which isn't something we'd want.

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

Finally, works on try server by XPCOM api.

https://tbpl.mozilla.org/?tree=Try&rev=d948636d9715

log (search test_ril_code_quality)
https://tbpl.mozilla.org/php/getParsedLog.php?id=24145556&tree=Try&full=1

03:52:27     INFO -  TEST-START test_ril_code_quality.py
03:52:54     INFO -  test_RILContenHelper (test_ril_code_quality.TestRILCodeQuality) ... ok
03:53:39     INFO -  test_RadioInterfaceLayer (test_ril_code_quality.TestRILCodeQuality) ... FAIL
03:54:12     INFO -  test_ril_consts (test_ril_code_quality.TestRILCodeQuality) ... ok
03:54:29     INFO -  test_ril_worker (test_ril_code_quality.TestRILCodeQuality) ... ok
03:54:30     INFO -  ======================================================================
03:54:30     INFO -  FAIL: test_RadioInterfaceLayer (test_ril_code_quality.TestRILCodeQuality)
03:54:30     INFO -  ----------------------------------------------------------------------
03:54:30     INFO -  Traceback (most recent call last):
03:54:30     INFO -    File "/builds/slave/test/build/tests/marionette/tests/dom/system/gonk/tests/marionette/test_ril_code_quality.py", line 287, in test_RadioInterfaceLayer
03:54:30     INFO -      self.go('RadioInterfaceLayer.js')
03:54:30     INFO -    File "/builds/slave/test/build/tests/marionette/tests/dom/system/gonk/tests/marionette/test_ril_code_quality.py", line 266, in go
03:54:30     INFO -      self.assertTrue(ok, error_message)
03:54:30     INFO -  AssertionError:
03:54:30     INFO -  RadioInterfaceLayer.js: line 3219, col 80, ['authtype'] is better written in dot notation.
03:54:30     INFO -  RadioInterfaceLayer.js: line 3223, col 56, ['authtype'] is better written in dot notation.
03:54:30     INFO -  TEST-UNEXPECTED-FAIL | test_ril_code_quality.py TestRILCodeQuality.test_RadioInterfaceLayer | See errors above

Comment 6

[Szu-Yu Chen [:aknow] (inactive)]

The test run the code quality check on 4 js files.
It got some warning for RadioInterfaceLayer.js and show the detailed information in |AssertionError|


TEST-START test_ril_code_quality.py
test_RILContenHelper (test_ril_code_quality.TestRILCodeQuality) ... ok
test_RadioInterfaceLayer (test_ril_code_quality.TestRILCodeQuality) ... FAIL
test_ril_consts (test_ril_code_quality.TestRILCodeQuality) ... ok
test_ril_worker (test_ril_code_quality.TestRILCodeQuality) ... ok
======================================================================
FAIL: test_RadioInterfaceLayer (test_ril_code_quality.TestRILCodeQuality)
----------------------------------------------------------------------
AssertionError:
RadioInterfaceLayer.js: line 3219, col 80, ['authtype'] is better written in dot notation.
RadioInterfaceLayer.js: line 3223, col 56, ['authtype'] is better written in dot notation.
TEST-UNEXPECTED-FAIL | test_ril_code_quality.py TestRILCodeQuality.test_RadioInterfaceLayer | See errors above

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1: Add a test for coding style check on RIL code by JSHint.

Please check the README first and maybe you would like to ignore the review of `jshint.js` because it's just a downloaded file.

Comment 8

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2: Address the suggestion from jshint

Comment 9

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=b8fb704c7b24

Comment 10

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1#2: Add a test for coding style check on RIL code by JSHint.

modify some comments

Comment 11

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#2: Address the suggestion from jshint

rebase mc

Comment 12

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1#3: Add a test for coding style check on RIL code by JSHint.

Comment 13

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#3: Address the suggestion from jshint

Comment 14

[Szu-Yu Chen [:aknow] (inactive)]

Hi Hsinyi,

Now ok to use 

switch (foo) {
  case 1: {
    break;  // ok to use 'break' in curly brace. I fix it in jshint.
  }
}

Comment 15

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

(In reply to Szu-Yu Chen [:aknow] from comment #14)
> Hi Hsinyi,
> 
> Now ok to use 
> 
> switch (foo) {
>   case 1: {
>     break;  // ok to use 'break' in curly brace. I fix it in jshint.
>   }
> }

I tried it. It works good. Thank you.
I'll review it tomorrow. Sorry for the delay :(

Comment 16

[Szu-Yu Chen [:aknow] (inactive)]

Add a live document
https://hackpad.com/Code-Quality-Test-For-RIL-Javascript-Code-In-Gecko-cz5j7YIGiw8

Comment 17

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1#4: Add a test for coding style check on RIL code by JSHint.

Comment 18

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: document

Comment 19

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

Comment on attachment 774442 [details] [diff] [review]
Part 1#4: Add a test for coding style check on RIL code by JSHint.

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

Awesome! I don't have more comments on the technical part (your slides rock!), except several grammatical errors. I tried to point some them out. Please help check them.

Before we landing the patch, let's send out a notification to dev-b2g, saying that we are going to have this terrific automation test, and telling everybody the rules.

::: dom/system/gonk/tests/marionette/ril_jshint/jshintrc
@@ +110,5 @@
> +        "gSystemWorkerManager": false,
> +        "gTimeService": false,
> +        "gUUIDGenerator": false,
> +        "ppmm": true,
> +        "z__end_guardian_for_easy_sorting__": false

What's "z__end_guardian_for_easy_sorting__"? I greped it but found nothing.

::: dom/system/gonk/tests/marionette/test_ril_code_quality.py
@@ +1,2 @@
> +'''
> +The test performs the static code analysis check by JSHint.

nit: s/check/checked

@@ +7,5 @@
> +  RadioInterfaceLayer.js
> +  ril_worker.js
> +  ril_consts.js
> +
> +If the js file contains the line of 'importScript()' (Ex: ril_worker.js). The

nit: s/. The/, the

@@ +18,5 @@
> +  importScripts('Script B')
> +  ...
> +  --------------------------------
> +
> +We combine these two scripts into one by following way.

nit: s/combine/merge
s/following/the following

@@ +26,5 @@
> +  (function(){ [Script A (ex: ril_worker.js)]
> +  })();
> +  --------------------------------
> +
> +Script A (ril_worker.js) has global strict mode.

nit: s/has/runs

@@ +29,5 @@
> +
> +Script A (ril_worker.js) has global strict mode.
> +Script B (ril_consts.js) not.
> +
> +The above merge way ensure the correct scope of 'strict mode'

nit: s/ensure/ensures
nit: s/strict mode'/strict mode.'

@@ +42,5 @@
> +import unicodedata
> +
> +
> +# ====================
> +# Some string utility.

String utilities.

@@ +72,5 @@
> +    lines[-1] += '\n'
> +  return lines
> +
> +def autoWrapStrictMode(lines):
> +  # For simple, we add the header in the beginning of the first line.

nit: s/simple/simplicity

@@ +101,5 @@
> +  return new_result_lines
> +
> +def errorsToLines(errors, filename = ''):
> +  '''
> +  Convert error object to readable string.

nit: Convert 'an' error object to 'a' readable string.

@@ +170,5 @@
> +      return global.channel.URI.spec;
> +    '''
> +    spec = self.runjs(code_open_channel)
> +
> +    # Make sure it is a jar uri, and not recursive.

nit: s/it/'spec'

@@ +173,5 @@
> +
> +    # Make sure it is a jar uri, and not recursive.
> +    #   Ex: 'jar:file:///system/b2g/omni.ja!/modules/ril_worker.js'
> +    #
> +    # For simplicity, we don't handle other special cases in this tests.

nit: s/tests/test

@@ +174,5 @@
> +    # Make sure it is a jar uri, and not recursive.
> +    #   Ex: 'jar:file:///system/b2g/omni.ja!/modules/ril_worker.js'
> +    #
> +    # For simplicity, we don't handle other special cases in this tests.
> +    # If build system change in the future, maybe put the jar in another jar,

If B2G build system  changes in the future, such as put one jar into another,

@@ +205,5 @@
> +    dst_line = 1
> +    dst_results = []
> +    info = []
> +
> +    # Put imported script first, and then itself.

nit: Put 'the' imported script first ...

'itself' looks ambiguous from the context. Could you try to make the comment clearer?

@@ +207,5 @@
> +    info = []
> +
> +    # Put imported script first, and then itself.
> +    for f in import_list + [filename]:
> +      # Mark merge from [file f, line 1] to [result, line dst_line]

Not really got this comment ...

Comment 20

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

Comment on attachment 771993 [details] [diff] [review]
Part 2#3: Address the suggestion from jshint

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

Thanks for the catch!

Comment 21

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1#5: Add a test for coding style check on RIL code by JSHint.

Re-organize the code. Let it share some structure with my command-line-version tool.
https://github.com/aknow/jshint-gecko/blob/master/gecko/jshint-gecko

In addition, reformat the code based on python coding style PEP 8
Please have a look. Thanks.

Comment 22

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

Comment on attachment 778326 [details] [diff] [review]
Part 1#5: Add a test for coding style check on RIL code by JSHint.

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

Thank you.

::: dom/system/gonk/tests/marionette/test_ril_code_quality.py
@@ +338,5 @@
> +
> +    def tearDown(self):
> +        MarionetteTestCase.tearDown(self)
> +
> +    def test_RILContenHelper(self):

s/RILContenHelper/RILContentHelper

Comment 23

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 1: Add a test for coding style check on RIL code by JSHint. r=hsinyi

Comment 24

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 2: Address the suggestion from jshint. r=hsinyi

Comment 25

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

http://hg.mozilla.org/projects/birch/rev/3d682482e90c
http://hg.mozilla.org/projects/birch/rev/bc5f5ce22ae3

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out for Marionette failures.
https://hg.mozilla.org/projects/birch/rev/7476377d143f

https://tbpl.mozilla.org/php/getParsedLog.php?id=25906379&tree=Birch

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Also, can you change this so that the error is outputted on the same line as AssertionError: so that the TBPL log parser can properly notice it?

i.e.
AssertionError: RadioInterfaceLayer.js: line 1896, col 9, 'gSmsService' is not defined.

Comment 28

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 1#2: Add a test for coding style check on RIL code by JSHint. r=hsinyi

Comment 29

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27)
> Also, can you change this so that the error is outputted on the same line as
> AssertionError: so that the TBPL log parser can properly notice it?
> 
> i.e.
> AssertionError: RadioInterfaceLayer.js: line 1896, col 9, 'gSmsService' is
> not defined.

Comment 30

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27)
> Also, can you change this so that the error is outputted on the same line as
> AssertionError: so that the TBPL log parser can properly notice it?
> 
> i.e.
> AssertionError: RadioInterfaceLayer.js: line 1896, col 9, 'gSmsService' is
> not defined.

There might be multiple warnings and cannot fit into one line. Ex:

AssertionError:
RadioInterfaceLayer.js: line 3219, col 80, ['authtype'] is better written in dot notation.
RadioInterfaceLayer.js: line 3223, col 56, ['authtype'] is better written in dot notation.
TEST-UNEXPECTED-FAIL | test_ril_code_quality.py TestRILCodeQuality.test_RadioInterfaceLayer | See errors above ...

What kind of information does TBPL log parser require?

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Can you put an AssertionError: before each warning? The TBPL log parser keys off that.

Comment 32

[Szu-Yu Chen [:aknow] (inactive)]

Ryan,

Current result is
==================
04:50:54     INFO -  AssertionError:
04:50:54     INFO -  RadioInterfaceLayer.js: line 1896, col 9, 'gSmsService' is not defined.
04:50:54     INFO -  TEST-UNEXPECTED-FAIL | test_ril_code_quality.py TestRILCodeQuality.test_RadioInterfaceLayer | See errors above and more information in Bug 880643
==================

'Assertion Error' is indeed before all the warnings. However TBPL log parser makes a link to the line of 'TEST-UNEXPECTED-FAIL', which is the last line of error message.

When I pass a multi-line message to assert method, the last line of the message will be place on the same line of 'TEST-UNEXPECTED-FAIL'

I don't know how to show the log after assert method.

Comment 33

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 1#3: Add a test for coding style check on RIL code by JSHint. r=hsinyi

Comment 34

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 1#4: Add a test for coding style check on RIL code by JSHint. r=hsinyi

... modify alignment

Comment 35

[Szu-Yu Chen [:aknow] (inactive)]

Try results for central/inbound/birch
https://tbpl.mozilla.org/?tree=Try&rev=f23d6f008301
https://tbpl.mozilla.org/?tree=Try&rev=398da671c5a3
https://tbpl.mozilla.org/?tree=Try&rev=d6dff8be9bfa

Comment 36

[Hsin-Yi Tsai (she/her) away [:hsinyi]]

Try looks good.

https://hg.mozilla.org/projects/birch/rev/9fa65f1d3803
https://hg.mozilla.org/projects/birch/rev/144bc645eee3

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9fa65f1d3803
https://hg.mozilla.org/mozilla-central/rev/144bc645eee3