820566 - DMD: Robustify and document the test

Status: RESOLVED FIXED

(Core :: DMD, defect)

Description

[Nicholas Nethercote [inactive]]

Now that DMD has landed, test-mode needs to be documented and made more robust, so that it doesn't only pass on my machine.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: DMD: Improve testing by using a script to filter out platform-specific stuff in the output.

This patch adds a new script, filter-test-output.py, which is used to strip
out platform-specific stuff from the output of test mode.  The new workflow is
to (a) run DMD in test mode, and (b) run the script, which tells you if the
output is acceptable or not.

(Ideally we'd have a |make check| target that does this in a single step, but
this gets us much closer to that ideal, and will do for now.)

Benefits:

- Fewer test-mode-only special cases in DMD.cpp.

- We now have the full output from test mode available, which is often useful
  to inspect when something goes wrong.

- (Some) stack frame records are now checked.  Previously they were just
  omitted in test mode.

- The test workflow exercises the fix*stacks scripts, which makes the testing
  workflow closer to the normal use workflow.

BTW, I'm a Python newbie, so I may well be doing sub-optimal things in the
script.

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 696218 [details] [diff] [review]
DMD: Improve testing by using a script to filter out platform-specific stuff in the output.

Review of attachment 696218 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/replace/dmd/filter-test-output.py
@@ +25,5 @@
> +
> +sysname = platform.system()
> +if sysname == "Linux":
> +    fix = "tools/rb/fix-linux-stack.pl"
> +elif sys == "Darwin":

I just tested on Mac, and |sys| should be |sysname|.

Comment 3

[Justin Lebar (not reading bugmail)]

Comment on attachment 696218 [details] [diff] [review]
DMD: Improve testing by using a script to filter out platform-specific stuff in the output.

r=me, but I hope you don't mind my nitting on the Python.  I'm happy to take
another look at the script if you'd like.

>diff --git a/memory/replace/dmd/filter-test-output.py b/memory/replace/dmd/filter-test-output.py
>new file mode 100755
>--- /dev/null
>+++ b/memory/replace/dmd/filter-test-output.py

I prefer hyphens to underscores in general, but Python does not play well with
hyphens in filenames; if you use hyphens, then you cannot import this module
with |import filter-test-output|.  So Python style is to use underscores.
(Consistent with e.g. "fix_macosx_stack.py".)

>@@ -0,0 +1,115 @@
>+#! /usr/bin/python
>+#
>+# This script takes the file produced by DMD's test mode and checks its
>+# correctness.
>+#
>+# Input file:   test.dmd
>+# Output files: test-fixed.dmd, test-filtered.dmd, test-diff.dmd.
>+#
>+# It runs the appropriate fix* script to get nice stack traces.  It also
>+# filters out platform-specific details from the test.dmd file.
>+#

I think it would be more consistent with how other *nix tools work if this
script accepted a filename as argv[2] instead of expecting a file in the cwd
called test.dmd.

Python style is to place this comment inside a docstring:

  """This script takes ...

  filters out platform-specific details from the test.dmd file.

  """

This way, the comment is available as this module's __doc__ attribute.

(We do the same thing for classes and functions.)

>+from __future__ import print_function
>+import os
>+import platform
>+import re
>+import subprocess
>+import sys

Canonical Python style for a script is to do

  def do_stuff():
    stuff

  if __name__ == '__main__':
    do_stuff()

instead of simply

  stuff

The reason is that one may want to import this module (even if only into the
Python shell, for testing).  As written, you can't do anything useful with this
script inside the shell, because it runs as soon as you import it.

>+#----------------------------------------------------------------------------
>+# Fix stack traces
>+#----------------------------------------------------------------------------
>+
>+print("fixing output to test-fixed.dmd...")
>+
>+sysname = platform.system()
>+if sysname == "Linux":
>+    fix = "tools/rb/fix-linux-stack.pl"
>+elif sys == "Darwin":
>+    fix = "tools/rb/fix_macosx_stack.py"
>+else:
>+    print("unhandled platform: " + sysname, file=sys.stderr)
>+    sys.exit(1)
>+
>+subprocess.call(fix, stdin=open("test.dmd", "r"),
>+                     stdout=open("test-fixed.dmd", "w"))

You should probably check the return value, or otherwise use check_call.

This assumes that the cwd is the root of the objdir.  At the very least, you
should document this (and perhaps print a helpful error message if you can't
find the fix* script).  It would also be helpful if you'd detect the difference
between being at the root of the srcdir and the root of the objdir, since
there's a tools/rb/fix-linux-stack.pl in both cases, but only the latter will
work properly.

>+#----------------------------------------------------------------------------
>+# Filter output
>+#----------------------------------------------------------------------------
>+
>+# In stack trace records we filter out all stack frames that contain a
>+# function whose name doesn't begin with "RunTestMode".  And the remaining
>+# ones have their line numbers omitted, unfortunately, because they are often
>+# off by one or two and this can vary between builds (e.g. debug vs
>+# non-debug).

This seems rather fortunate to me; I don't want to have to update the test
every time I touch DMD.cpp!  :)

>+# As for stack frame records, we complete eliminate all those that contain a
>+# function whose name doesn't begin with "RunTestMode", because such stack
>+# frame records are highly dependent on the exact forms of stack traces.
>+
>+print("filtering output to test-filtered.dmd...")
>+
>+fin  = open("test-fixed.dmd",    "r")
>+fout = open("test-filtered.dmd", "w")
>+
>+test_frame_re = re.compile(r".*(RunTestMode\w*).*(DMD.cpp)")
>+
>+for line in fin:
>+    if re.match(r" (Allocated at|Reported( again)? at)", line):
>+        # It's a stack trace record.
>+        print(line, end='', file=fout)
>+
>+        # Filter the stack trace -- only show RunTestMode* frames.
>+        for frame in fin:
>+            if re.match(r"   ", frame):

r"    " is the same as "    ".

>+                m = test_frame_re.match(frame)
>+                if m:
>+                    print("   ...", m.group(1), "...", m.group(2), file=fout)
>+            else:
>+                # We're past the stack trace.
>+                print(frame, end='', file=fout)
>+                break
>+
>+    elif re.search("in stack frame record", line):
>+        # Stack frame record.  Get the whole thing (we know how many lines it
>+        # has).
>+        line2 = fin.next()
>+        line3 = fin.next()
>+        line4 = fin.next()
>+        frame = fin.next()
>+        line6 = fin.next()
>+        m = test_frame_re.match(frame)
>+        if m:
>+            # This is a stack frame record from RunTestMode* -- print it,
>+            # obscuring record numbers (which vary unpredictably).
>+            print(re.sub(r"record \d+ of \d+", "record M of N", line),
>+                  end='', file=fout)
>+            print(line2, end='', file=fout)
>+            print(line3, end='', file=fout)
>+            print(line4, end='', file=fout)
>+            print("   ...", m.group(1), "...", m.group(2), file=fout)
>+            print(line6, end='', file=fout)
>+
>+    else:
>+        # Some line that doesn't need special handling.  Copy it through.
>+        print(line, end='', file=fout)
>+
>+fout.flush()

You should probably close() fout.

>+#----------------------------------------------------------------------------
>+# Check against expected output
>+#----------------------------------------------------------------------------
>+
>+print("diffing output to test-diff.dmd")
>+
>+subprocess.call(["diff", "-u",
>+                 "test-filtered.dmd", "memory/replace/dmd/test-expected.dmd"],

test-expected.dmd gets placed into the objdir?

>+                stdout=open("test-diff.dmd", "w"))

>+if os.stat("test-diff.dmd").st_size == 0:
>+    print("test passed")
>+else:
>+    print("test failed")

If you check the return value of diff, you don't have to do this.  You could
also use difflib.unified_diff, if you wanted to.

ISTM that a nicer way of doing this would be to use temp files (in /tmp; see
the tempfile module) for everything except the diff output, which you'd write
to stdout.  In most cases, we don't care about anything other than the diff,
and then you wouldn't end up cluttering the objdir with test result files.

You could add a command-line option not to delete the temp files, for debugging
purposes.

Perhaps you don't even need temp files, if you keep everything in memory.

Comment 4

[Nicholas Nethercote [inactive]]

Thanks for the Python tips!


> So Python style is to use underscores.
> (Consistent with e.g. "fix_macosx_stack.py".)

Done.

 
> I think it would be more consistent with how other *nix tools work if this
> script accepted a filename as argv[2] instead of expecting a file in the cwd
> called test.dmd.

Done;  see below.

 
> Python style is to place this comment inside a docstring:
> 
>   """This script takes ...

Done.


> Canonical Python style for a script is to do
> 
>   def do_stuff():
>     stuff
> 
>   if __name__ == '__main__':
>     do_stuff()

Done.


> This assumes that the cwd is the root of the objdir.

Not quite.  It assumes that (a) the cwd is the dir that Firefox was invoked from, because that's where test.dmd will be, and (b) the cwd is the srcdir, because it hard-codes memory/replace/dmd/test-expected.dmd.

I've changed it so you specify the srcdir and the test.dmd file on the command line.  This lifts restriction (a) and (b).  However, if you want it to actually work, restriction (a) remains because test.dmd contains relative paths;  I've documented this.


> It would also be helpful if you'd detect the difference
> between being at the root of the srcdir and the root of the objdir, since
> there's a tools/rb/fix-linux-stack.pl in both cases, but only the latter will
> work properly.

Both fix scripts are identical and will work fine.  (I've been using the one in srcdir.)


> This seems rather fortunate to me; I don't want to have to update the test
> every time I touch DMD.cpp!  :)

To get around that problem, my plan was to move the test code into a separate file and #include it.  But no need for that now.

 
> r"    " is the same as "    ".

Yes, but it seems like a good idea to always use raw strings in |re| methods...


> You should probably close() fout.

I used a |with| statement to auto-close it, which allowed me to remove the flush().

 
> >+if os.stat("test-diff.dmd").st_size == 0:
> >+    print("test passed")
> >+else:
> >+    print("test failed")
> 
> If you check the return value of diff, you don't have to do this.

True!  I've done that.


> ISTM that a nicer way of doing this would be to use temp files (in /tmp; see
> the tempfile module) for everything except the diff output, which you'd write
> to stdout.  In most cases, we don't care about anything other than the diff,
> and then you wouldn't end up cluttering the objdir with test result files.

From experience, I want two things:
- a succinct success/fail indicator;
- on failure, the diff in a file.

So I won't do the stdout thing.  But I have changed it to put the generated files in /tmp.


> Perhaps you don't even need temp files, if you keep everything in memory.

It can be useful to look at temp files, so I won't do that.

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdfc52d62d52

Comment 6

[Justin Lebar (not reading bugmail)]

How can fix_linux_stack.pl work if it's invoked with cwd=srcdir?  It needs to do addr2line on the object files.

Comment 7

[Nicholas Nethercote [inactive]]

Object files, or libraries?  The library paths in the outputs on both my machines (Linux and Mac) are absolute.  But if that's not universally true, it might explain why you had test failures on B2G.

Comment 8

[Justin Lebar (not reading bugmail)]

> The library paths in the outputs on both my machines (Linux and Mac) are absolute.

Ah, right!  Okay, carry on.

(I think the problem I was having when I tried to run the DMD test on B2G was different -- I wasn't getting any stacks at all, for some reason.)

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fdfc52d62d52