518126 - IPDL: Need regression tests

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

After bug 518032, which was an idiotic regression, I'm beginning to get quite concerned over the lack of IPDL regression tests.  The reason I've been slow to implement them is that it's pretty hard to test an in-flux code generator directly, and indirectly testing some of the subtleties of IPDL semantics is quite hard.  But even just a set of 10 or so sanity checks would do a world of good.  High in my priority queue, especially as regression tests block bug 506171.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1, v1: test the IPDL compiler in isolation

r? because the patch includes some changes to the build system, though they're quite straightforward.  Please advise if the r? is unnecessary.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pushed http://hg.mozilla.org/projects/electrolysis/rev/43bda478500c, part 1.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2, v1: test framework for IPDL-generated C++ code

There's only one example test now, TestSanity.  Writing C++ tests is more involved than writing IPDL-compiler-only tests.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 402472 [details] [diff] [review]
part 2, v1: test framework for IPDL-generated C++ code

Whoops, a bunch of problems with this patch.  New one coming right up.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2, v2 --- now with all the new files hg add'd!

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2, v2.5 --- build ipc/ipdl/test/cxx later than the rest of ipc/

Clobber build failed, had to dance around a little to get the build order right.  Last canceled review, I promise.

Comment 7

[Benjamin Smedberg]

Comment on attachment 402513 [details] [diff] [review]
part 2, v2.5 --- build ipc/ipdl/test/cxx later than the rest of ipc/

>diff --git a/ipc/ipdl/test/cxx/Makefile.in b/ipc/ipdl/test/cxx/Makefile.in

>+# TODO: we should only recurse into this directory for TEST builds, as
>+# we compile code here that's eventually linked into libxul

That's no good. Our release builds enable tests (so that we can package the tests separately and run them on the release builds). Why does this need to be linked into libxul?

(Even if this were the case, you'd use ifdef MOZ_ENABLE_TESTS to condition it).

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2, v3: only enable IPDL C++ tests for --enable-ipdl-tests builds

Temporary compromise per IRC discussion.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 402618 [details] [diff] [review]
part 2, v3: only enable IPDL C++ tests for --enable-ipdl-tests builds

diff --git a/ipc/ipdl/Makefile.in b/ipc/ipdl/Makefile.in
-
-DIRS += test
-

Why did you move this to the tier_dirs? If it's just for linkage reasons, you can make this TOOL_DIRS instead (TOOL_DIRS get built after everything else.)

diff --git a/ipc/ipdl/test/cxx/IPDLUnitTests.h b/ipc/ipdl/test/cxx/IPDLUnitTests.h

I think for consistency's sake, you should consider using the same "passed" and "fail" functions available in TestHarness.h:
http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestHarness.h

diff --git a/ipc/ipdl/test/cxx/Makefile.in b/ipc/ipdl/test/cxx/Makefile.in
+EXPORTS_mozilla/_ipdltest =			\
+	IPDLUnitTests.h				\
+	IPDLUnitTestThreadChild.h		\
+	$(NULL)

Don't use tabs for continuations, just use two-space indents. (Use tabs only for the commands in rules.) Ditch the other tabs you have laying around too, except in rule commands.

+# Define this so that IPDLUnitTests.cpp isn't the default make target
+all::

Alternately, you should be able to just move all your specific rules down below the include of rules.mk.

+IPDLUnitTests.cpp : $(IPDLTESTHDRS) $(GENTESTER)
+	$(PYTHON) $(GENTESTER) $(IPDLTESTS) > $@

If you make $(GENTESTER) the first prereq, then you can refer to it as $< in the command.

+RUNIPDLTEST := $(DEPTH)/dist/bin/run-mozilla.sh $(DEPTH)/dist/bin/ipdlunittest$(BIN_SUFFIX)

You need to use $(RUN_TEST_PROGRAM) instead of calling run-mozilla.sh directly, since it doesn't exist or work on Windows.

diff --git a/ipc/ipdl/test/cxx/app/Makefile.in b/ipc/ipdl/test/cxx/app/Makefile.in
+NSDISTMODE = copy

Why are you using this?

diff --git a/ipc/ipdl/test/cxx/genIPDLUnitTests.py b/ipc/ipdl/test/cxx/genIPDLUnitTests.py
+if __name__ != '__main__':
+    print >>sys.stderror, 'Can only be run as "main" script'

We tend to prefer that build scripts are importable as modules, envisioning a future where PyMake imports them. Could you uplift all your code into a main() method, then just write:
if __name__ == '__main__:
  main(sys.argv)
? That'd make me happier.

+sys.stdout.write(string.Template(

That is a pretty huge chunk of C++ code to have stuffed inside your Python script. Could you pull that out into a .cpp.template file or something, read it in, and then format from there? I feel like that might preserve some sanity in the future.

diff --git a/toolkit/library/libxul-config.mk b/toolkit/library/libxul-config.mk
+ifdef MOZ_IPDL_TESTS
+STATIC_LIBS += ipdlunittest_s
+endif

Perhaps I missed the discussion, but why does this need to get linked into libxul? That's pretty ugly.

I'm not reviewing the C++ bits here (aside from the test harness header), I presume bsmedberg has OKed them already.

r=me with those things addressed.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2, v4: address ted's comments

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pushed http://hg.mozilla.org/projects/electrolysis/rev/32b33070fcf5