604347 - selftest.as : generate one .cpp rather than many

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tools, enhancement, P3)

Description

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

Currently adding new selftests is far more painful [1] than adding new ActionScript tests.

Lars suggests that funneling all of the selftest code into one .cpp, which is then the only file referenced by the build infrastructure, would address this. selftest.as could automatically introduce C++ namespaces to keep the selftests from stepping on each others' toes.

[1] Main pain point: there are too many projects to update etc.  Bug 579144 has an aborted attempt to address that, which was abandoned because it was not sufficiently general.

Comment 1

[Brent Baker]

QRB: QE would really like to have this issue targeted and resolved quickly. As we are working on increasing code coverage, there is a need to add selftests and the cost of adding a new selftest at the moment is huge (see point [1] above).

Comment 2

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

(swipe!)

Comment 3

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

Attachment: simple(istic) solution to putting all st content into one file.

This may be an overly simplistic solution to the problem.

Note in particular that it does not attempt to isolate the selftests in their own namespaces, which means that file-static functions may collide.  (The system was already vulnerable to collisions and remains so; I'm just pointing out a new one that arises with this approach.)  I do plan to put in automatic namespace-based isolation of selftests, but some of them might be relying on a lack of such isolation, and we can probably resolve the problem of this ticket directly without adding that feature right now.

Comment 4

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

Comment on attachment 490629 [details] [diff] [review]
simple(istic) solution to putting all st content into one file.

brbaker: FYI my intent in posting this is for you to give it a spin in the spots where QE would be using it and see if it mostly solves your problems.

Comment 5

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

Attachment: simple(istic) solution to putting all st content into one file.

(small revision; I accidentally left the manifest deltas out of the last patch.  Though of course you need to update the other project files as well.)

Comment 6

[Brent Baker]

Comment on attachment 490634 [details] [diff] [review]
simple(istic) solution to putting all st content into one file.

Have not tried out the patch yet but quick feedback (will provide additional feedback if necessary after using patch):
 
I think that we should be sorting the list of *.st entries in System.argv instead of leaving it up to the os to give the files in any order that it desires (I believe that windows will expand to case insensitive order but linux will be case sensitive), this will cause unnecessary diffs in the generated file. (Order should be guaranteed by the script and not up to the OS)

I also assume that the final/push version of this will include the removal of the old generated .cpp files and the addition of the new one (along with project file updates)

Comment 7

[Brent Baker]

Comment on attachment 490634 [details] [diff] [review]
simple(istic) solution to putting all st content into one file.

Should remove the following from formatSelftest() since the file is now generated from all of the files and is recorded properly with the prefix_note

@@ -281,17 +286,16 @@ package selftest
             return s.replace(/\"/g, "\\\"");
         }
     }
 
     function formatSelftest(input, st) {
         var s = [];
         var classname = "ST_" + st.component + "_" + st.category;
         st.classname = classname;
-        s.push("// Generated from " + input);
         pushMultiple(s, st.comments);
         s.push("#include \"avmshell.h\"");
         st.ifdef_text = null;
         if (st.ifdefs.length > 0)
             st.ifdef_text = "#if defined " + st.ifdefs.join(" && defined ");
         if (st.ifndefs.length > 0)
             st.ifndef_text = "#if !defined " + st.ifndefs.join(" && !defined ");
         s.push("#ifdef VMCFG_SELFTEST");

Comment 8

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

(In reply to comment #7)
> Comment on attachment 490634 [details] [diff] [review]
> simple(istic) solution to putting all st content into one file.
> 
> Should remove the following from formatSelftest() since the file is now
> generated from all of the files and is recorded properly with the prefix_note

I think I was deliberately leaving those notes in place.  The remaining commented out "#line" annotations might serve a similar purpose, but then again it can be useful to have markers at the borders.

Comment 9

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

Comment on attachment 490634 [details] [diff] [review]
simple(istic) solution to putting all st content into one file.

In case anyone's curious, this code is out-of-date and will not work with the current collection of selftests.  in particular, i think the vmbase concurrency selftests are exercising corners of C++ that were not used at the time this patch was written.  (we're not even talking about particularly obscure corners of C++ here.  Its just a fragile patch.)

Comment 10

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

Attachment: wrap each selftest in its own namespace

To my surprise, this relatively simple change actually seems to work.

Or at least, it compiles on my mac and the selftests seem to run and still pass.

So we might be able to put this in long before gyp lands.  :)

Comment 11

[Brent Baker]

Comment on attachment 528732 [details] [diff] [review]
wrap each selftest in its own namespace

I checked locally and via the sandbox, and the change successfully compiled and ran the selftests.

I assume that the additional house-keeping will be done in the actual commit:
- removing ST_*.cpp
- updating all necessary VS and xcode project files to remove knowledge about ST_*.cpp and add SelftestExec.cpp

Comment 12

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

(In reply to comment #11)
> Comment on attachment 528732 [details] [diff] [review]
> wrap each selftest in its own namespace
> 
> I checked locally and via the sandbox, and the change successfully compiled and
> ran the selftests.
> 
> I assume that the additional house-keeping will be done in the actual commit:
> - removing ST_*.cpp
> - updating all necessary VS and xcode project files to remove knowledge about
> ST_*.cpp and add SelftestExec.cpp

Yep.  (Plus, I plan to sort the arguments to selftest.as, as you suggested in comment 6; I added that in last night but it wasn't part of the patch I posted.)

Comment 13

[Tamarin Bot]

changeset: 6256:644d9cfe0ab1
user:      Felix S Klock II <[e-mail]>
summary:   Bug 604347: all st code in one file w/ namespace guards (r=lhansen, f+brbaker).

http://hg.mozilla.org/tamarin-redux/rev/644d9cfe0ab1