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

RESOLVED FIXED in mozilla25

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aknow, Assigned: aknow)

Tracking

Trunk
mozilla25
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(3 attachments, 12 obsolete attachments)

233 bytes, text/html
Details
2.57 KB, patch
Details | Diff | Splinter Review
322.05 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 759688 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
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
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 1

5 years ago
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)
(Assignee)

Comment 3

5 years ago
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)

Updated

5 years ago
Assignee: nobody → szchen
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 5

5 years ago
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
(Assignee)

Comment 6

5 years ago
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
(Assignee)

Updated

5 years ago
Attachment #759688 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Created attachment 765764 [details] [diff] [review]
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.
Attachment #765764 - Flags: review?(htsai)
(Assignee)

Comment 8

5 years ago
Created attachment 765765 [details] [diff] [review]
Part 2: Address the suggestion from jshint
Attachment #765765 - Flags: review?(htsai)
(Assignee)

Comment 10

5 years ago
Created attachment 770083 [details] [diff] [review]
Part 1#2: Add a test for coding style check on RIL code by JSHint.

modify some comments
Attachment #765764 - Attachment is obsolete: true
Attachment #765764 - Flags: review?(htsai)
Attachment #770083 - Flags: review?(htsai)
(Assignee)

Comment 11

5 years ago
Created attachment 770084 [details] [diff] [review]
Part 2#2: Address the suggestion from jshint

rebase mc
Attachment #765765 - Attachment is obsolete: true
Attachment #765765 - Flags: review?(htsai)
Attachment #770084 - Flags: review?(htsai)
(Assignee)

Comment 12

5 years ago
Created attachment 771992 [details] [diff] [review]
Part 1#3: Add a test for coding style check on RIL code by JSHint.
Attachment #771992 - Flags: review?(htsai)
(Assignee)

Comment 13

5 years ago
Created attachment 771993 [details] [diff] [review]
Part 2#3: Address the suggestion from jshint
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)
(Assignee)

Comment 14

5 years ago
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 :(
(Assignee)

Comment 17

5 years ago
Created attachment 774442 [details] [diff] [review]
Part 1#4: Add a test for coding style check on RIL code by JSHint.
(Assignee)

Comment 18

5 years ago
Created attachment 774445 [details]
document
(Assignee)

Updated

5 years ago
Attachment #774445 - Attachment filename: file_880643.txt → file_880643.html
Attachment #774445 - Attachment mime type: text/plain → text/html
(Assignee)

Updated

5 years ago
Attachment #771992 - Attachment is obsolete: true
Attachment #771992 - Flags: review?(htsai)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 21

5 years ago
Created attachment 778326 [details] [diff] [review]
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.
Attachment #774442 - Attachment is obsolete: true
Attachment #778326 - Flags: review?(htsai)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 23

5 years ago
Created attachment 782975 [details] [diff] [review]
[Final] Part 1: Add a test for coding style check on RIL code by JSHint. r=hsinyi
Attachment #778326 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
Created attachment 782976 [details] [diff] [review]
[Final] Part 2: Address the suggestion from jshint. r=hsinyi
Attachment #771993 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 28

5 years ago
Created attachment 783405 [details] [diff] [review]
[Final] Part 1#2: Add a test for coding style check on RIL code by JSHint. r=hsinyi
Attachment #782975 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 29

5 years ago
(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
(Assignee)

Comment 30

5 years ago
(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.
(Assignee)

Comment 32

5 years ago
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.
(Assignee)

Comment 33

5 years ago
Created attachment 783557 [details] [diff] [review]
[Final] Part 1#3: Add a test for coding style check on RIL code by JSHint. r=hsinyi
Attachment #783405 - Attachment is obsolete: true
(Assignee)

Comment 34

5 years ago
Created attachment 783558 [details] [diff] [review]
[Final] Part 1#4: Add a test for coding style check on RIL code by JSHint. r=hsinyi

... modify alignment
Attachment #783557 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9fa65f1d3803
https://hg.mozilla.org/mozilla-central/rev/144bc645eee3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.