Closed Bug 518126 Opened 10 years ago Closed 10 years ago

IPDL: Need regression tests

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
r? because the patch includes some changes to the build system, though they're quite straightforward.  Please advise if the r? is unnecessary.
Attachment #402204 - Flags: review?(benjamin)
Attachment #402204 - Flags: review?(benjamin) → review+
There's only one example test now, TestSanity.  Writing C++ tests is more involved than writing IPDL-compiler-only tests.
Attachment #402472 - Flags: review?(benjamin)
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.
Attachment #402472 - Attachment is obsolete: true
Attachment #402472 - Flags: review?(benjamin)
Clobber build failed, had to dance around a little to get the build order right.  Last canceled review, I promise.
Attachment #402507 - Attachment is obsolete: true
Attachment #402513 - Flags: review?(benjamin)
Attachment #402507 - Flags: review?(benjamin)
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).
Attachment #402513 - Flags: review?(benjamin) → review-
Temporary compromise per IRC discussion.
Attachment #402513 - Attachment is obsolete: true
Attachment #402618 - Flags: review?(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.
Attachment #402618 - Flags: review?(ted.mielczarek) → review+
Attachment #402618 - Attachment is obsolete: true
Attachment #404665 - Flags: review?(benjamin)
Attachment #404665 - Flags: review?(benjamin) → review+
Pushed http://hg.mozilla.org/projects/electrolysis/rev/32b33070fcf5
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.