Closed Bug 880643 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: aknow, Assigned: aknow)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(3 files, 12 obsolete files)

233 bytes, text/html
Details
2.57 KB, patch
Details | Diff | Splinter Review
322.05 KB, patch
Details | Diff | Splinter Review
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.
Summary: Add a code quality test on try server for ril javascript code in gecko → [B2G RIL] Add a code quality test on try server for ril javascript code in gecko
Summary: [B2G RIL] Add a code quality test on try server for ril javascript code in gecko → B2G RIL: Add a code quality test on try server for ril javascript code in gecko
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.
Flags: needinfo?(jgriffin)
From a test running in the build, this file is available at the url "resource://gre/modules/ril_worker.js"; is that sufficient?
Flags: needinfo?(jgriffin)
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.
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.
Assignee: nobody → szchen
Summary: B2G RIL: Add a code quality test on try server for ril javascript code in gecko → B2G RIL: Add a code quality test on try server for RIL javascript code in gecko
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
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
Attachment #759688 - Attachment is obsolete: true
Please check the README first and maybe you would like to ignore the review of `jshint.js` because it's just a downloaded file.
Attachment #765764 - Flags: review?(htsai)
Attachment #765765 - Flags: review?(htsai)
modify some comments
Attachment #765764 - Attachment is obsolete: true
Attachment #765764 - Flags: review?(htsai)
Attachment #770083 - Flags: review?(htsai)
rebase mc
Attachment #765765 - Attachment is obsolete: true
Attachment #765765 - Flags: review?(htsai)
Attachment #770084 - Flags: review?(htsai)
Attachment #770083 - Attachment is obsolete: true
Attachment #770084 - Attachment is obsolete: true
Attachment #770083 - Flags: review?(htsai)
Attachment #770084 - Flags: review?(htsai)
Attachment #771993 - Flags: review?(htsai)
Hi Hsinyi,

Now ok to use 

switch (foo) {
  case 1: {
    break;  // ok to use 'break' in curly brace. I fix it in jshint.
  }
}
(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 :(
Attached file document
Attachment #774445 - Attachment filename: file_880643.txt → file_880643.html
Attachment #774445 - Attachment mime type: text/plain → text/html
Attachment #771992 - Attachment is obsolete: true
Attachment #771992 - Flags: review?(htsai)
Attachment #774442 - Flags: review?(htsai)
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 ...
Attachment #774442 - Flags: review?(htsai) → review+
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!
Attachment #771993 - Flags: review?(htsai) → review+
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.
Attachment #774442 - Attachment is obsolete: true
Attachment #778326 - Flags: review?(htsai)
Attachment #778326 - Attachment description: Part 1#54: Add a test for coding style check on RIL code by JSHint. → Part 1#5: Add a test for coding style check on RIL code by JSHint.
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
Attachment #778326 - Flags: review?(htsai) → review+
Keywords: checkin-needed
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.
Keywords: checkin-needed
(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.
Keywords: checkin-needed
(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?
Can you put an AssertionError: before each warning? The TBPL log parser keys off that.
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9fa65f1d3803
https://hg.mozilla.org/mozilla-central/rev/144bc645eee3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.