Improve Code Quality of RIL Code by JSHint

Transcript

1. Improve Code Quality of RIL Code by JSHint aknow@mozilla 2013.06
2. Target audience • All RIL code committer ◦ RILContentHelper.js ◦ RadioInterfaceLayer.js ◦ ril_worker.js ◦ ril_consts.js
3. Note • I Add some materials at the end of the slide ◦ 2013.07 • Please finish all the slide before performing the install action ◦ Something different ◦ Otherwise, you might waste your time
4. Why lint? • Lint tool ◦ Flag suspicious usage in software written in any computer language • Suspicious usage include ◦ variables being used before being set ◦ division by zero ◦ conditions that are constant ◦ ...
5. For C++ in google • coding style ◦ http://google-styleguide.googlecode. com/svn/trunk/cppguide.xml • lint tool: cpplint ◦ http://google-styleguide.googlecode. com/svn/trunk/cpplint/ ◦ An automated checker to make sure a C++ file follows Google's C++ style guide
6. Why on RIL code? • Try to find the bug A missing comma ','
7. And ... it results in • A power on failure on target device • You check the log ◦ realize that it's a javascript syntax error ◦ fix the error ◦ rebuild ◦ reflash ◦ ... • How about there are more than one error?
8. How SW Engineers spend their time
9. So... Don't waste your time • Use linting tool ◦ Find those silly errors • Most of RIL code are written in javascript ◦ Need a js linting tool
10. State-of-the-art -- JSHint • JSLint ◦ http://www.jslint.com/ ◦ Written by Douglas Crockford -- a master on javascript • JSHint ◦ http://www.jshint.com/ ◦ Fork from JSLint but a community-driven tool ◦ More configurable ◦ Can easily adjust it to your particular coding guidelines
11. Two Stages • Use JSHint on local environment ◦ Guarantee quality of code written by you • Add it as a test case ◦ On try server ◦ Guarantee quality of code all the time in repository
12. Use JSHint on local environment • As a node program • Run from command line ◦ $ jshint <js-file>
13. Install nodejs • nodejs ◦ js runtime based on v8 ◦ similar to xpc shell, but much more famous • install ◦ download the tar file from http://nodejs.org/ ▪ $ ./configure ▪ $ make ▪ $ [sudo] make install • You may also use the nvm
14. Test your node env
15. Install JSHint • Install globally ◦ ref: http://www.jshint.com/install/ ◦ $ npm -g install jshint
16. Config JSHint • Config the rules ◦ enforce or relax • Provide globals that defined in other files • My configuration ◦ https://raw.github.com/aknow/ril_jshint/master/.jshintrc ◦ Put in project root or home ~/.jshintrc
17. Workaround 1: fall through in switch • JSHint detect => /* falls through */ • We want to change it to
18. Workaround 1: fall through in switch .. (2) • Modification ◦ <path to node module>/jshint/src/stable/reg.js ▪ <path ...> = /usr/local/lib/node_modules if you install jshint globally ◦ Change the regex ▪ exports.fallsThrough = /^\s*\/\/\s*Falls? \sthrough.*\s*$/;
19. Workaround 2: let foo = function foo() {}; • Current limitation of JSHint ◦ Regards as an error in JSHint. ◦ // 'foo' is already defined and can't be redefined • A lot of this kinds of usage in RIL code.
20. Workaround 2: let foo = function foo() {}; .. (2) • Modification ◦ <path to node module>/jshint/src/stable/jshint.js ◦ Search E044, and remove entire block • Drawback ◦ No longer detect the redefined variable issue ◦ Unless we modify the function name ◦ ... or fix this issue in JSHint
21. Workaround 3: break in switch case block • a know issue for JSHint before fixing, code this way
22. Workaround 4: importScripts() • in ril_worker.js • $ jshint ril_woker.js ◦ cannot handle importScript() ◦ 1500 errors ◦ a lot of undefined symbol that defined in ril_consts.js
23. Workaround 4: importScripts() .. (2) • Provide a wrapper command ◦ https://raw.github.com/aknow/ril_jshint/master/jshintx ◦ put in your PATH ◦ $ jshintx ril_worker.js
24. Behind the wrapper command importScripts('ril_consts.js') ril_worker.js [from ril_worker.js] tmp.js [from ril_consts.js] merge the scripts $ jshint tmp.js tmp.js line xxx tmp.js line ooo tmp.js line ### Error results map the results back ril_consts.js line xxx ril_worker.js line ooo ril_worker.js line ### correct filename and line number
25. Detailed of merge way [from ril_worker.js] [from ril_consts.js] (code from ril_consts.js) ... ... (code from ril_worker.js) ... "use strict"; simple concate global "use strict"; should be declare before any statement [from ril_worker.js] [from ril_consts.js] (code from ril_worker.js) ... "use strict"; importScripts(...) (code from ril_consts.js) ... (code from ril_worker.js) simple nested the code in ril_consts.js now affected by "use strict";
26. Correct merge way [from ril_worker.js] [from ril_consts.js] (code from ril_consts.js) ... (function() { (code from ril_worker.js) ... "use strict"; importScripts(...) ... })(); concate with scope ril_consts.js: no strict ril_worker.js: has strict Not guarantee correct for general cases, but works for ril_worker.js
27. Two Stages • Use JSHint on local environment ◦ Guarantee quality of code written by you • Add it as a test case ◦ On try server ◦ Guarantee quality of code all the time in repository
28. General Concept • Use browser bundle of JSHint ◦ as a function ◦ var ok = JSHINT(source, options, globals) • Through marionette testing framework ◦ load jshint.js, execute by marionette ▪ Get the function JSHINT() ◦ load source code, ex: ril_worker.js ▪ Create a string for entire source code ◦ execute JSHINT(<source>, .., ..) function then get the results
29. Marionette test case in python • Basic ◦ Everytime, marionette create a new js environment to run the script • To keep the previous execution results
30. Now create the test case • Steps a. Read jshint.js and execute it b. Read source code, ex: ril_worker.js c. Convert it to a string d. Execute
31. a. Read jshint.js and execute it • Challenge: how to locate jshint.js ◦ know the location of test_ril_code_quality.py ◦ then relative path <location>/ril_jshint/jshint.js • python inspect ◦ http://docs.python.org/2/library/inspect.html ◦ inspect.getfile(TestRILCodeQuality) ▪ know where the class is defined dom/system/gonk/tests
33. b. Read source code ex: ril_worker.js • Challenge: how to locate the source file ◦ Easy: use the same technique, relative path ▪ Works well on local but fail on try server. ▪ Actually this is the hardest part I think when developing this test case • This is the location of test case on try server ◦ /builds/slave/test/build/tests/marionette/tests/dom/sy stem/gonk/tests/marionette/test_ril_code_quality.py ▪ try server makes the tests package first • test_ril_code_quality.py is moved • different with what we test on local
34. b. Read source code ex: ril_worker.js .. (2) /builds/slave/test/build/tests/marionette/tests/dom/system/g onk/tests/marionette/test_ril_code_quality.py • Search on try server by brute-force ◦ like IDDFS (Iterative deepening depth-first search) root / ... marionette 1st search 2nd search nth search while not found search in current folder go up to parent folder timeout no output in 20mins, try will kill the test
35. b. Read source code ex: ril_worker.js .. (3) • Use XPCOM api ◦ resource URI: resource://gre/modules/ril_worker.js ◦ Cannot direct open it as a web page • Open it as a channel by IOService ◦ jar:file:///system/b2g/omni.ja!/modules/ril_worker.js ◦ ... impossible to found ril_woker.js on try server ▪ it's in a jar (zip) file
36. b. Read source code ex: ril_worker.js .. (4) • Map from jarURI to local file • Open the file by zip reader • Extract the entry • Read it
37. get exception if the return object has some empty field inside
38. C. Convert it to a string • Challenge: string escape ◦ In python, marionette.execute_script( <a> ) ▪ you need a python string <a> ◦ <a>'s content is a javascript: JSHINT(<b>, ..., ...) ◦ <b> is a javascript string for your source code; • Example: ◦ Your JS code: ▪ var a=' he say: " hello "'; ▪ var b ◦ You want to lint this js code ◦ how could you write in python ▪ marionette.execute_script('JSHINT(...)')
39. C. Convert it to a string .. (2) • Your JS code: ◦ in string => 'var a=\' he say: " hello "\';\nvar b' ▪ var a=' he say: " hello "'; ▪ var b • In test ◦ 'JSHINT(\'var a=\\\' he say: " hello "\\\';\\nvar b\')'
40. C. Convert it to a string .. (3)
41. d. Execute • Done • Just get the result and show when assert fail
42. Summary • Why? ◦ Improve the code quality ◦ Detect syntax error and save your time ◦ Should not let you feel there are more restrictions • How? ◦ Local develop environment ▪ install nodejs, jshint ▪ use provided .jshintrc ▪ before you use modify jshint for workaround 1, 2 (fall through, let redefine) ▪ use provided wrapper command => jshintx
43. Summary .. (2) • How? ◦ As marionette test case (also enabled on try server) ▪ you can run the test on local • $ ./test.sh marionette <path-to- gecko>/dom/system/gonk/tests/marionette/test_ril_code_qual ity.py ▪ don't worry for the fail, just check the log • https://tbpl.mozilla.org/php/getParsedLog.php? id=24224554&tree=Try&full=1 • What? Bug 880643 ◦ B2G RIL: Add a code quality test on try server for RIL javascript code in gecko
44. 2013.07 Update • Setup local environment for jshint ◦ https://github.com/aknow/jshint-gecko/ ◦ One-step installation ◦ All workaround 1, 2, 3, 4 are proper handled ▪ fall through => fixed in jshint ▪ let redefine => fixed in jshint ▪ switch case => fixed in jshint ▪ importScript() => fixed in jshint-gecko