707086 - runtests.py incorrectly determining 0 from abcdump runs

Status: RESOLVED FIXED

(Tamarin Graveyard :: Build Config, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Deep tests are failing.

Output is many fails of the form:

2588 running as3/Definitions/Classes/Override/IntExtDefaultOverRideProtected.as  
   FAILED! :
Expected (from abcdump): 0 functions verified
GOT (-Dverifyall -Dverbose=verify): 87 functions verified
   FAILED passes:0 fails:1 unexpected passes: 0 expected failures: 0

Expecting "0 functions verified" seems bogus.  From what I can tell, the root problem here is that the abcdump.abc does not exist, and is not being rebuilt (or at least signalling a failure due to its absence).

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #0)
> From what I can tell, the
> root problem here is that the abcdump.abc does not exist, and is not being
> rebuilt (or at least signalling a failure due to its absence).

No, this diagnosis was bogus on two counts.  (The abcdump.abc *does* exist, and the runtestBase.py *does* check for it.)

Will continue looking.

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Ah, I think my supposed refactorings for Bug 706799 are related to this; I must have changed the behavior on well-formatted abcs even though that was not my intent.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Appears to be injected by:

Tamarin Bot 2011-12-01 07:14:57 PST

changeset: 6761:58a27bbda05d
user:      Felix S Klock II <[e-mail]>
summary:   Bug 706799: refactor all constant pool accesses into helpers (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/58a27bbda05d

(I thought I had seen another instance of a problem like this before I starting landing my refactorings.  But maybe I imagined that, or maybe its unrelated.)

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Aha, in
  http://hg.mozilla.org/tamarin-redux/rev/58a27bbda05d
these code transformations were valid:

 -                    namespaces[i] = new ABCNamespace(strings[readU32()], nskind)
 +                    namespaces[i] = new ABCNamespace(stringFromU32(), nskind)

but these code transformations were bogus:


 -                            s += abc.namespaces[readU32()]
 +                            s += abc.namespaceFromU32()

because the readU32() on the abc object is distinct from the readU32() in the local scope.

(I had vaguely thought about this when I first wrote the code but convinced myself that they would be equivalent; untrue.)

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Of course, it would be nice if the non-zero error-exit status of the abcdump run were somehow reflected in the runtests.py output.

I'll fix the core problem to unblock the deep builds first though.

Comment 6

[Tamarin Bot]

changeset: 6762:948e3694ac20
user:      Felix S Klock II <[e-mail]>
summary:   Bug 707086: revert portion of changeset 6761:58a27bbda05d (Bug 706799) that broke abcdump (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/948e3694ac20

Comment 7

[Tamarin Bot]

changeset: 6763:ad451f02f6ee
user:      Felix S Klock II <[e-mail]>
summary:   Bug 707086: missed another case that should have been part of changeset 6762:948e3694ac20 (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/ad451f02f6ee

Comment 8

[Tamarin Bot]

changeset: 6764:06e841b6adef
user:      Felix S Klock II <[e-mail]>
summary:   Bug 707086: fix typo in changeset 6763:ad451f02f6ee (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/06e841b6adef

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch E v1: runtests.py check abcdump error exit code

Stashing this patch here; it does the job but is really ugly.

IMO the "right" way to do this would be to throw an exception, but my experiments with trying to add that ran up against my lack of knowledge of Python.  (I thought I added such an exception, but it seemed like it was being ignored and the corresponding test was simply ignored/skipped by runtests.py.  I don't know if that's by design or an oversight in the runtests.py code.)

(This code in general seems like it could really use a round of refactoring; there's a ton of patterns that could be factored into helper methods with self-describing names.)