499016 - lirasm - Provide testing support for lirasm

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Paritosh Aggarwal]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11) Gecko/2009060308 Ubuntu/9.04 (jaunty) Firefox/3.0.11
Build Identifier: 

A testing mechanism is required to ensure modifications made to the source don't introduce bugs/still produce correct output on all the tests.


Reproducible: Always

Comment 1

[Paritosh Aggarwal]

Attachment: Adds testlirc.sh and few test cases

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 383799 [details] [diff] [review]
Adds testlirc.sh and few test cases

Great, thanks for the patch.

>   if [[ `$LIR_ASM "$ASM_FLAGS" $LIR_FILE` && -z `$LIR_ASM "$ASM_FLAGS" $LIR_FILE|diff - $LIR_RES` ]]

I can't tell what this is trying to do.  It seems like the way to do this would be like:

  if "$LIR_ASM" $ASM_FLAGS "$LIR_FILE" | cmp -s - "$LIR_RES"

Or maybe:

  if "$LIR_ASM" $ASM_FLAGS "$LIR_FILE" > tmpfile
      && cmp -s "$LIR_RES" tmpfile

You have some extra indentation on lines 18-19.

Comment 3

[Jason Orendorff [:jorendorff]]

Also, you would of course get bonus points if "make check" in an objdir ran the tests.

Comment 4

[Jason Orendorff [:jorendorff]]

Sorry for the spam-- another obstacle is that lirasm currently has different output in debug vs. release builds.  I think we should change it so that the verbose output is an option (when NJ_VERBOSE is defined).

Comment 5

[Graydon Hoare :graydon]

(In reply to comment #4)
> Sorry for the spam-- another obstacle is that lirasm currently has different
> output in debug vs. release builds.  I think we should change it so that the
> verbose output is an option (when NJ_VERBOSE is defined).

Agreed. You probably want to look at the initialization path for the new logging code that's landing in bug 494864.

Comment 6

[Paritosh Aggarwal]

Attachment: Revised patch with testing support, make check option

Hopefully the make check is done correctly.
Fixed the other recommendations you suggested above.

Comment 7

[Paritosh Aggarwal]

Attachment: Revised patch with testing and make check

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: Paritosh's patch with some changes

This is the same as Paritosh's patch but with these changes:

1. Move all the files to the source directory, and changes needed after that to keep "make check" working.

2. General tidying up in the shell script.

3. Change "make check" so it doesn't build lirasm (I think *not* having a dependency "check:: program_being_tested" is customary for us.)

4. Don't include tests that won't run yet (we should add them in time).

There are some tests that don't pass yet, but that will be fixed in bug 497991.

Comment 9

[Paritosh Aggarwal]

Attachment: Above patch with clean(without debug spew) tests and option to display debug output

Same as above patch with tests without the debug spew, and an option to display debug spew only when required. That is, default behaviour now changes to display required output only.

Comment 10

[Paritosh Aggarwal]

A couple of additional changes included in the above patch:
 - Remove support for LIR_calli and LIR_fcalli, since those have been removed.
 - Add a rm testout.txt line in testlirc.sh, to clean up temporary file.

Comment 11

[Graydon Hoare :graydon]

Comment on attachment 384124 [details] [diff] [review]
Above patch with clean(without debug spew) tests and option to display debug output

Looks good. Also consider as a followup testing task a disassembly tester that objdumps the s-record results (static output, not by executing the code) and compares against an expected objdump. I wrote an example test script in the original lirasm bug but did not commit it.

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a66962120961