800612 - Enable compiled code tests in content/canvas

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

In Bug 732660, we added a first compiled-code test in a new directory, content/canvas/test/compiled.

We ran into the problem that they were being linked before libxpcom was linked, which caused linking to fail.

At that time, it seemed that replacing TEST_DIRS by TOOLS_DIRS in the Makefile was a way to solve this issue.

However, today I realized that that caused the content/canvas mochitests to not be run at all.

So what is the real way to enable compiled-code tests there? If TOOLS_DIRS is really the way, then that means that we should take compiled/ out of content/canvas/test.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Seems like TOOLS_DIRS was just a typo for TOOL_DIRS *sigh*

https://tbpl.mozilla.org/?tree=Try&rev=22f197df1767

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Test failure on Windows. Help please!

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Help needed to understand this cppunittest failure!

Comment 4

[(not currently active) Ted Mielczarek]

There's some bad error handling in the Python script there, but it sounds like it's probably failing to load some DLL that that test requires. I changed how C++ tests were run when I landed this runcppunittests.py script, they now get run from the directory they were built in, but with LD_LIBRARY_PATH or PATH set to point to dist/bin so they can load the rest of Gecko (that's what the --xre-path environment variable is for).

You can try running the test manually with something like:
PATH=e:/builds/moz2_slave/try-w32-dbg/build/obj-firefox/dist/bin:$PATH MOZ_XRE_DIR=e:/builds/moz2_slave/try-w32-dbg/build/obj-firefox/dist/bin e:/builds/moz2_slave/try-w32-dbg/build/obj-firefox/content/canvas/test/compiled/TestWebGLElementArrayCache.exe

If that doesn't give you any more information you can try running it under something like Dependency Walker: http://www.dependencywalker.com/ which should be able to show you what the error is.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: use TOOL_DIRS, and move compiled tests to a separate directory

This moves the compiled tests to a separate directory alongside test/ . So we no longer have both mochitests and compiled tests under the same directory. This seems to work, at least it built on Windows.

https://tbpl.mozilla.org/?tree=Try&rev=9a98775c1681

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: use TOOL_DIRS, and move compiled tests to a separate directory

Adapts a makefile to this directory move. Explains above build failures. New try:
https://tbpl.mozilla.org/?tree=Try&rev=909b464a40d2

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

You need to wrap the TOOL_DIRS in  ifdef ENABLE_TESTS

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Thanks. Also, the above try run discovered a real issue in the code, thanks to Windows' rand() function returning different values than Linux's with the same seed. Looking into it. Upping the number of repeats allows to reproduce on Linux.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: fix WebGLElementArrayCache bug and increase test repetitions to catch this bug

See comment in the patch. The fix here was quite obvious from reproducing this in GDB. we were clamping lastByte but weren't clamping firstByte. Rather than clamping firstByte, a more general fix was to add this early-return path in the no-op case where firstByte > lastByte after clamping lastByte.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: use TOOL_DIRS, and move compiled tests to a separate directory

Thanks Ms2ger for the tip.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=bea311c1a4e9

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Question: if I change something in content/canvas/compiledtest/TestWebGLElementArrayCache.cpp and make -C objdir/content/canvas/compiledtest, it doesn't rebuild the test, it only re-runs it. That's a bit annoying. make -C objdir/content/canvas does rebuild the test. What's wrong and how can I get make -C compiledtest to rebuild the test?

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Another question: the Android logs in comment 11 suggest that the test wasn't actually run on Android. It was run on Windows. What's wrong?

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Oh, I suppose that as on Android we cross-compile, we can't run the compiled tests as part of the build job. So how can I see Android compiled tests on TBPL?

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 675865 [details] [diff] [review]
use TOOL_DIRS, and move compiled tests to a separate directory

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

::: content/canvas/test/compiled/Makefile.in
@@ +11,5 @@
>  
>  include $(DEPTH)/config/autoconf.mk
>  
>  LOCAL_INCLUDES := \
> +    -I$(srcdir)/../src \

You could fix this to 2-space indent while you're here.

Comment 16

[(not currently active) Ted Mielczarek]

(In reply to Benoit Jacob [:bjacob] from comment #12)
> Question: if I change something in
> content/canvas/compiledtest/TestWebGLElementArrayCache.cpp and make -C
> objdir/content/canvas/compiledtest, it doesn't rebuild the test, it only
> re-runs it. That's a bit annoying. make -C objdir/content/canvas does
> rebuild the test. What's wrong and how can I get make -C compiledtest to
> rebuild the test?

I thought we fixed this in bug 777379. Did that not actually fix it?

(In reply to Benoit Jacob [:bjacob] from comment #14)
> Oh, I suppose that as on Android we cross-compile, we can't run the compiled
> tests as part of the build job. So how can I see Android compiled tests on
> TBPL?

We don't currently run C++ unit tests on Android. They're only run as part of "make check" on non-cross-compiled platforms.

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d0cb3b68e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a31d3b784446

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7d0cb3b68e0
https://hg.mozilla.org/mozilla-central/rev/a31d3b784446

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Oops, the "increase test repeats" part failed to apply and i hadn't noticed. Good thing I checked test logs.

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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