Closed
Bug 880643
Opened 11 years ago
Closed 11 years ago
B2G RIL: Add a code quality test on try server for RIL javascript code in gecko
Categories
(Core :: DOM: Device Interfaces, defect)
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.
Assignee | ||
Updated•11 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•11 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•11 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)
Comment 2•11 years ago
|
||
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•11 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.
Comment 4•11 years ago
|
||
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•11 years ago
|
Assignee: nobody → szchen
Assignee | ||
Updated•11 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•11 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•11 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•11 years ago
|
Attachment #759688 -
Attachment is obsolete: true
Assignee | ||
Comment 7•11 years ago
|
||
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•11 years ago
|
||
Attachment #765765 -
Flags: review?(htsai)
Assignee | ||
Comment 9•11 years ago
|
||
Assignee | ||
Comment 10•11 years ago
|
||
modify some comments
Attachment #765764 -
Attachment is obsolete: true
Attachment #765764 -
Flags: review?(htsai)
Attachment #770083 -
Flags: review?(htsai)
Assignee | ||
Comment 11•11 years ago
|
||
rebase mc
Attachment #765765 -
Attachment is obsolete: true
Attachment #765765 -
Flags: review?(htsai)
Attachment #770084 -
Flags: review?(htsai)
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #771992 -
Flags: review?(htsai)
Assignee | ||
Comment 13•11 years ago
|
||
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•11 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.
}
}
Comment 15•11 years ago
|
||
(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 16•11 years ago
|
||
Assignee | ||
Comment 17•11 years ago
|
||
Assignee | ||
Comment 18•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #774445 -
Attachment filename: file_880643.txt → file_880643.html
Attachment #774445 -
Attachment mime type: text/plain → text/html
Assignee | ||
Updated•11 years ago
|
Attachment #771992 -
Attachment is obsolete: true
Attachment #771992 -
Flags: review?(htsai)
Assignee | ||
Updated•11 years ago
|
Attachment #774442 -
Flags: review?(htsai)
Comment 19•11 years ago
|
||
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 20•11 years ago
|
||
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•11 years ago
|
||
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•11 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 22•11 years ago
|
||
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•11 years ago
|
||
Attachment #778326 -
Attachment is obsolete: true
Assignee | ||
Comment 24•11 years ago
|
||
Attachment #771993 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 25•11 years ago
|
||
http://hg.mozilla.org/projects/birch/rev/3d682482e90c
http://hg.mozilla.org/projects/birch/rev/bc5f5ce22ae3
Keywords: checkin-needed
Whiteboard: [fixed-in-birch]
Comment 26•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
Attachment #782975 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 29•11 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•11 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?
Comment 31•11 years ago
|
||
Can you put an AssertionError: before each warning? The TBPL log parser keys off that.
Assignee | ||
Comment 32•11 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•11 years ago
|
||
Attachment #783405 -
Attachment is obsolete: true
Assignee | ||
Comment 34•11 years ago
|
||
... modify alignment
Attachment #783557 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 35•11 years ago
|
||
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•11 years ago
|
||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 37•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9fa65f1d3803
https://hg.mozilla.org/mozilla-central/rev/144bc645eee3
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in
before you can comment on or make changes to this bug.
Description
•