tntim96 / JSCover

JSCover is a JavaScript Code Coverage Tool that measures line, branch and function coverage
GNU General Public License v2.0
399 stars 83 forks source link

Javascript syntax differences #154

Closed miller-m closed 10 years ago

miller-m commented 10 years ago

I think our javascipt is significantly different than the javascript that JSCover is expecting. We are stuffing our functions inside of JSONs. Don't ask me why, it was like that before I got here, and we have roughly 450k lines of javascript already.

Here's a typical file we have:

https://github.com/forcedotcom/aura/blob/master/aura-components/src/main/components/ui/actionCheckbox/actionCheckboxController.js

When I instrument the code and try to run our tests on it (using the command mvn verify -DrunIntTests) our JSON parsers are going crazy. When I looked at the instrumented code, it is not similar at all. We've kicked around some ideas for having to fix the files for us to work with. Here's what we came up with:

read-and-emit until you find the "function BranchData() {" line, then insert a header: ({ superInit: function() { then emit the BranchData line and continue read-and-emitting until you get a lineData[[0-9]]++; line. Eat that, eat the following (which should be ({ plus whitespace), then continue read-and-emit until either you find init (and insert the check and maybe call superInit) or you reach }), in which case you emit a skeleton init and then close.

What do you think? I've looked at the proxy method, but am unsure of how that actually works. I see https://github.com/tntim96/JSCover-samples/blob/master/src/test/java/jscover/webdriver/proxy/WebDriverGeneralProxyTest.java but don't know if that replaces a class in jscover, if I specify that when running the proxy server, or if I just run it as a standalone class. If you remember, our tests start a jetty application server and then use chromedriver to run the tests.

tntim96 commented 10 years ago

We've kicked around some ideas for having to fix the files for us to work with

I'll have to investigate that. Feel free to fork and try it yourself. There are plenty of tests to help.

I've looked at the proxy method, but am unsure of how that actually works

I thought you were going to use file-system instrumentation in which case you can run your chromedriver tests as normal as mentioned in https://github.com/tntim96/JSCover-maven-plugin/issues/7

miller-m commented 10 years ago

My thought behind the proxy was that because it's instrumenting the javascript on the fly, that might work around the js formatting issues I mentioned in the first part of the email.

On Tue, Jul 22, 2014 at 2:45 AM, tntim96 notifications@github.com wrote:

We've kicked around some ideas for having to fix the files for us to work with

I'll have to investigate that. Feel free to fork and try it yourself. There are plenty of tests to help.

I've looked at the proxy method, but am unsure of how that actually works

I thought you were going to use file-system instrumentation in which case you can run your chromedriver tests as normal as mentioned in tntim96/JSCover-maven-plugin#7 https://github.com/tntim96/JSCover-maven-plugin/issues/7

— Reply to this email directly or view it on GitHub https://github.com/tntim96/JSCover/issues/154#issuecomment-49718246.

tntim96 commented 10 years ago

The instrumentation will be very similar and have the same issue. Instrumenting on the file-system will allow you to try the your idea. You first run JSCover, then run your post-JSCover formatting step. It looks like it can be done via string manipulation(rather than JS-AST manipulation before emission).

miller-m commented 10 years ago

yeah i've been playing around with it today. Currently our JSON parser doesn't like this line: return '"' + s.replace(/[\u0000-\u001f"\u007f-\uffff]/g, function (c) { saying unterminated string.

On Tue, Jul 22, 2014 at 2:26 PM, tntim96 notifications@github.com wrote:

The instrumentation will be very similar and have the same issue. Instrumenting on the file-system will allow you to try the your idea. You first run JSCover, then run your post-JSCover formatting step. It looks like it can be done via string manipulation(rather than JS-AST manipulation before emission).

— Reply to this email directly or view it on GitHub https://github.com/tntim96/JSCover/issues/154#issuecomment-49803585.

miller-m commented 10 years ago

Do you happen to know of anyone using JSCover who has JSONs stuffed with functions?

tntim96 commented 10 years ago

No. It doesn't seem to be a common practice: http://stackoverflow.com/questions/2001449/is-it-valid-to-define-functions-in-json-results

miller-m commented 10 years ago

Lets say I didn't care about branch coverage. I'm trying to make things as simple as possible so I can just figure out how many lines are covered. I tried the --no-coverage option but didn't see any changes in the instrumentation. What can I take out? I want to have a minimum of new functions included in the instrumented code that I'll have reformat.

Also, and this is going to show my inexperience with how javascript works, how does the code at the end of the instrumented file that's not a part of any function get executed?

Matt

On Wed, Jul 23, 2014 at 1:02 AM, tntim96 notifications@github.com wrote:

No. It doesn't seem to be a common practice: http://stackoverflow.com/questions/2001449/is-it-valid-to-define-functions-in-json-results

— Reply to this email directly or view it on GitHub https://github.com/tntim96/JSCover/issues/154#issuecomment-49844025.

tntim96 commented 10 years ago

Lets say I didn't care about branch coverage

Try --no-branch (and --no-function to turn off function coverage). It's documented in the file-mode section of the manual.

how does the code at the end of the instrumented file that's not a part of any function get executed

Not quite sure what you mean, but all global statements get executed as they are loaded.

miller-m commented 10 years ago

I have an actual possible bug to report. File name is too long when trying to save the coverage data. There's nothing to catch the exception.

20140725 12:06:34.266,5227,SEVERE,"Error saving coverage data",jscover.server.InstrumentingRequestHandler, java.lang.RuntimeException: java.io.FileNotFoundException: target/local-storage-proxy/original-src/l/%7B%22mode%22%3A%22SELENIUM%22%2C%22app%22%3A%22themeSanityTest%3AusingOverride%22%2C%22requestedLocales%22%3A%5B%22en_US%22%2C%22en%22%5D%2C%22loaded%22%3A%7B%22APPLICATION%40markup%3A%2F%2FthemeSanityTest%3AusingOverride%22%3A%22C6pJiLNBbtoADjezqJgNAw%22%7D%2C%22lastmod%22%3A%221406308699000%22%2C%22test%22%3A%22org.auraframework.impl.root.theme.ThemingSanityUITest.testThemeOverride%22%2C%22fwuid%22%3A%22kfy0rNaTZa2gHcqcsDbV8g%22%7D/app.js (File name too long) at jscover.util.IoUtils.copy(IoUtils.java:534) at jscover.util.IoUtils.copy(IoUtils.java:562) at jscover.server.InstrumentingRequestHandler.storeReport(InstrumentingRequestHandler.java:425) at jscover.server.InstrumentingRequestHandler.handlePostOrPut(InstrumentingRequestHandler.java:391) at jscover.server.HttpServer.run(HttpServer.java:418) Caused by: java.io.FileNotFoundException: target/local-storage-proxy/original-src/l/%7B%22mode%22%3A%22SELENIUM%22%2C%22app%22%3A%22themeSanityTest%3AusingOverride%22%2C%22requestedLocales%22%3A%5B%22en_US%22%2C%22en%22%5D%2C%22loaded%22%3A%7B%22APPLICATION%40markup%3A%2F%2FthemeSanityTest%3AusingOverride%22%3A%22C6pJiLNBbtoADjezqJgNAw%22%7D%2C%22lastmod%22%3A%221406308699000%22%2C%22test%22%3A%22org.auraframework.impl.root.theme.ThemingSanityUITest.testThemeOverride%22%2C%22fwuid%22%3A%22kfy0rNaTZa2gHcqcsDbV8g%22%7D/app.js (File name too long)

tntim96 commented 10 years ago

What version are you using? It doesn't appear to be the latest from that stack-trace.

Also, what mode are you using, server or proxy? If proxy, can you re-try with the latest version? If I decode those URL paths it might help.

Also, is that the correct URL? It's unusual target/local-storage-proxy/original-src/l/{"mode":"SELENIUM","app":"themeSanityTest:usingOverride","requestedLocales":["en_US","en"],"loaded":{"APPLICATION@markup://themeSanityTest:usingOverride":"C6pJiLNBbtoADjezqJgNAw"},"lastmod":"1406308699000","test":"org.auraframework.impl.root.theme.ThemingSanityUITest.testThemeOverride","fwuid":"kfy0rNaTZa2gHcqcsDbV8g"}/app.js

miller-m commented 10 years ago

What I ended up doing was replacing the extra long URL with a string that I count to increase it. I knew that I was going to have the same file over and over again. The URL is a bunch of hash values and settings for the dependencies involved.

At this point, I've kind of realized that our JavaScript library might not be able to have code coverage really make sense for it. We have our functions in JSONs like I mentioned, and when we go to build actual html and send the javascript to the client, the javascript is concatenated together into an app.js file. It determines what goes into that app.js file based on the dependencies coded in (basically import statements in our code). So our Javascript files aren't directly used by the client. They're parsed and then the specifically asked for JS is sent to the client in the app.js file.

What I could do is, for testing, say the components involved in the tests depend on EVERYTHING, but that seems like it's going to be massively overkill, with a 200 mb file being passed for every javascript test. As the app.js is the only javascript that gets executed by the client, that's the only way I get any line coverage.

If I wanted to see what individual javascript files are used in testing, I think I'm going to have to create my own tool for it. Basically, when js files are parsed, I'll have to add them to a static list of parsed files, then after all the integration tests are run, traverse the directory structure to find all JS files and compare to the list of parsed files. That way I'll know if there are any untested classes. I won't know line coverage though.

On Sat, Jul 26, 2014 at 1:14 AM, tntim96 notifications@github.com wrote:

What version are you using? It doesn't appear to be the latest from that stack-trace.

If I decode those URL paths it might help.

— Reply to this email directly or view it on GitHub https://github.com/tntim96/JSCover/issues/154#issuecomment-50223734.

tntim96 commented 10 years ago

It's a pity there's not a lower level of testing for these components. Perhaps you can add a layer that assembles your target JS surrounded by mocks and stubs for testing - that won't translate directly to your source but may be better than assembling the full JS.

Anyway, good luck.