425794 - Treehydra: Test Framework + Fix include filenames

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[David Mandelin [:dmandelin]]

Treehydra passes the wrong filename to JS in the native function Include. It should pass the real filename so that stack traces show the real path to the file.

Comment 1

[(dormant account)]

Should throw an exception if JS file can't be found

Comment 2

[David Mandelin [:dmandelin]]

Attachment: Proposed patch

Fixed the original issue + now throws exceptions on failures to find include file.

Comment 3

[(dormant account)]

Comment on attachment 312375 [details] [diff] [review]
Proposed patch

>+    // Doing a little extra work here to get rid of unneeded '/'.

I know I abuse every C rule in the code, but lets at least pretend that we use C comments.

Just fix this and commit.

Comment 4

[David Mandelin [:dmandelin]]

(In reply to comment #3)
> (From update of attachment 312375 [details] [diff] [review])
> >+    // Doing a little extra work here to get rid of unneeded '/'.
> 
> I know I abuse every C rule in the code, but lets at least pretend that we use
> C comments.

Heh. I try to remember to avoid those, because I knew you wouldn't like them. It turns out they are part of C99, I don't know what you think of C99. Anyway, I did fix them but then I noticed a possible functional issue with my patch. If the main script is not found, you get a message like this:

[dmandelin@dm-oink01 dev-thex]$ treehydra nofile.js a.cc
NULL:0: : Cannot find include file 'nofile.js'

Which is a little weird. Think I should make it say "treehydra: Cannot find script file 'nofile.js'" or some such?

Comment 5

[(dormant account)]

Fine, guess I'll stop using the crappy /* */ comments :)

Regarding the exception message, ideally you'd pass __FILE__, __LINE__ to spidermonkey, so it's obvious that it's thrown from C code.

Current message isn't that bad. I consider errors with file:line info to be a gift, so the fact that it's not there isn't that bad. Your call.

Comment 6

[David Mandelin [:dmandelin]]

Attachment: Revised patch

When an include fails, you get a JS exception, and the message is tagged with the C location:

    [dmandelin@dm-oink01 dev-thex]$ treehydra d.js a.cc
    d.js:1: JS Exception: Error: Cannot find include file 'asldkfjalsdf' (from dehydra.c:179)
    :0:	    #0: include("asldkfjalsdf")

When something fails at the top level, primarily when a nonexistent script is given:

    [dmandelin@dm-oink01 dev-thex]$ treehydra f.js a.cc
    dehydra.c:179: Error: Cannot find include file 'f.js'

I had to use a little bit of somewhat annoying trickery to get it so that you don't have to put __FILE__, __LINE__ every time you call reportError, but I don't know an obviously superior solution.

Comment 7

[David Mandelin [:dmandelin]]

Attachment: Unit tests for this patch

Attached are some unit tests for the given patch. This also represents the beginnings of a unit test framework for Treehydra. Some comments on the design:

1. Clearly, it's best to have unit tests written in the same language (or at least same execution platform) as the code being tested, so the checkers can walk the object data, etc. So, I wanted a unit test system for JS. There's something called jsUnit [1], but it looks like it's big (6 MB download) and designed for in-browser usage, so I just made something simple based off the jUnit-style APIs. test_include.js is a unit test of this style, and can be run as a Treehydra script.

2. Some things can't be done inside Treehydra JS, namely testing top-level error messages, which is important for this task. For those, I have a Python script that runs Treehydra and checks the output. I used the Python unit testing framework, not because it does a whole lot in the case, but so that the code would be compatible with a standard testing API.

3. This makes it look like there are 2 separate sets of tests: Python and JS. Actually, there will be more, because separate Treehydra scripts will necessarily run separately. So I have the Python script set up to run JS unit tests as well, so you can run it, and if it prints "OK: n tests passed", you know everything worked.

Comment 8

[(dormant account)]

a)
Nice stuff. However, I'm not sure if python is the right task for this job. I know that particular popen api wasn't available in on dm-oink at one point. 

Would it make more sense to factor out the js code(it sort of already is) and make a pure-js toplevel sort of like the js executable in spidermonkey. It would help us test spidermonkey better, that's my main preference for this approach.

On the other hand, I already require python in the configure script, so it's no big deal to use it.

b) How do you run this stuff, python test_scripterr.py gives me a lot of unhappy errors on dm-oink

Comment 9

[David Mandelin [:dmandelin]]

(In reply to comment #8)
> a)
> Nice stuff. However, I'm not sure if python is the right task for this job. I
> know that particular popen api wasn't available in on dm-oink at one point. 
> 
> Would it make more sense to factor out the js code(it sort of already is) and
> make a pure-js toplevel sort of like the js executable in spidermonkey. It
> would help us test spidermonkey better, that's my main preference for this
> approach.

My take was that running multiple Treehydra processes and collecting the results requires *something* with libraries for pipes, process control, etc., and that it would be a ton of otherwise unnecessary work to implement all that for Spidermonkey. But perhaps there is an easier way than what I imagined.

The pipe stuff shouldn't be any problem on dm-oink, it's just a matter of Python versions.

> On the other hand, I already require python in the configure script, so it's no
> big deal to use it.
> 
> b) How do you run this stuff, python test_scripterr.py gives me a lot of
> unhappy errors on dm-oink

I think all you need is a shell command 'treehydra' that takes 2 arguments, the script first and the C++ file second. See ~dmandelin/local/bin/treehydra.

I guess in this dir, that should be replaced with the correct paths based on the Makefile, probably have them passed into the test script via command line.

Comment 10

[David Mandelin [:dmandelin]]

Attachment: Combined revised patch: features + unit tests

Here's a combined patch. Hopefully, at this point 'make unit_tests' inside the test directory will run the tests and print 'OK', even if you are not me and do not have my shell scripts. :-)

I vote for doing unit testing within JS when possible, but keeping the existing simple Python unit test stuff for things that can't easily live within Treehydra scripts (e.g. top-level error reporting) and collating results of many separate Treehydra runs.  At least for now--fine with me if we file separate bugs for that, but I'd like to get this in so I can start fixing up and pushing in the dependent patches.

Comment 11

[David Mandelin [:dmandelin]]

Attachment: Fixed patch--missing files added in

Had some new files missing from the previous.

Comment 12

[(dormant account)]

I think the makefile integration is great.

However the output is less than ideal. Instead of backtraces, i'd like to see a list of commands that failed and a summary.

Comment 13

[David Mandelin [:dmandelin]]

(In reply to comment #12)
> I think the makefile integration is great.
> 
> However the output is less than ideal. Instead of backtraces, i'd like to see a
> list of commands that failed and a summary.
> 

I should be able to do that without too much difficulty just by formatting the output differently. 

Comment 14

[David Mandelin [:dmandelin]]

Attachment: Better unit test result reporting

Improved the error reporting. You still get a stack trace for non-test-assert exceptions, which are presumably (a) bugs in the test suite, or (b) too weird to know what error message to put up ahead of time. But I converted the exception with empty output from a JS unit test script to be a test-assert failure.

Here's an example of output with a failure:
---
Test Failure: 
    Test command: ../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_treehydra.so -o /dev/null -fplugin-arg=syntax_error.js empty.cc
    Failure msg: Expected 'SyntaxError' in error output; not found

Unit Test Suite Summary:
      4 passed
      1 failed
      0 error(s)

Comment 15

[(dormant account)]

Comment on attachment 313232 [details] [diff] [review]
Better unit test result reporting

Looks good.

I get:
2 passed 
3 failed

Is that expected? Your sample output looks different :)

Obviously we'll need to have a policy of 0 failures now. Fix these and commit.

Comment 16

[(dormant account)]

Nevermind, i forgot to make before running the test. Please commit.