Combine code-coverage scripts into a platform generic script

VERIFIED FIXED in Q3 11 - Serrano

Status

Tamarin
Build Config
P2
normal
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: Brent Baker, Assigned: Brent Baker)

Tracking

(Blocks: 1 bug)

unspecified
Q3 11 - Serrano
Bug Flags:
in-testsuite -
flashplayer-qrb +
flashplayer-bug -
flashplayer-triage +

Details

(Whiteboard: code-coverage)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Currently there are 3 copies of the code-coverage scripts (windows-deep, mac-deep, linux-deep) which are not unique. This makes it cumbersome to add new logic as it needs to be duplicated needlessly.

If the scripts are combined it will also ensure that all platforms are compiling and testing the sets of vmshells and testsuites.
Flags: in-testsuite-
Flags: flashplayer-triage+
Flags: flashplayer-qrb?

Updated

8 years ago
Assignee: nobody → brbaker
Status: NEW → ASSIGNED
Flags: flashplayer-qrb? → flashplayer-qrb+
Priority: -- → P2
Target Milestone: --- → flash10.2
(Assignee)

Comment 1

8 years ago
Current thought on how to do this is to have 2 common csv files that drive the compiling and the running of the shells. One CSV will describe each shell that is to be compiled and instrumented and the other describes how to run/test the instrumented shells. Having the data contained in CSV will make it easy to see exactly what we are compiling and testing and will be easy to integrate into the "Code Coverage" wiki page.

For both compiling and testing we will simply leverage the build/buildbot/slaves/all/*-generic scripts that the build system is already using.


CSV for building the shell:
format: display_name, shell_name, configure options, env_script, notes

display_name: [required] 
Description of shell, ex: Release, Release No JIT, Release sysmalloc

shell_name: [required, used to compile, needs to be unique] 
name of the shell, avmshell, avmshell_sd, coverage file will also be named based on this value <shell_name>.cov. Do not include the file extension

configure options: [required, used to compile] 
options that are passed into configure.py, ex: --enable-shell --enable-debugger

env_script: [required, used to compile] 
file that will be parsed to setup the environment for compilation. This is here mainly to facilitate compiling 32bit and 64bit shells on windows and linux.
On windows we will need to setup different paths (PATH, INCLUDE, etc) for 32bit/64bit and on linux will will need to setup different compiler switches to enable 32bit builds via 64bit compiler:
ex: CXX=g++ -m32
https://help.ubuntu.com/community/InstallingCompilers

notes: [optional] 
Free text that would clarify what the build is

Example:
Release32, avmshell, --enable-shell, env32, "Release 32bit"

DebugDebugger64, avmshell_sd_64, --enable-shell --enable-debug --enable-debugger, env64, "Debug Debugger 64bit"

ReleaseDebuggerAIR32, avmshell_air_s, --enable-shell --enable-override-global-new --enable-use-system-malloc --enable-debugger, env32, "Release Debugger with MMgc configured how AIR sets up MMgc" 


Open Issues:
[1] There are additional/different switches that need to be passed to configure.py based on the platform. Mac requires --mac-sdk=105 and to compile 64 vs. 32 bit shells we need to pass the --target= switch. Would it be possible for the 'env_script' to contain an additional entrry that would be appended to the configure.py args? This way the mac-deep/scripts/env64 could contain something like:
    configure_args="--target=x86_64-darwin --mac-sdk=105"
and the env32 would contain:
    configure_args="--mac-sdk=104u"

[2] Will we need to extend the csv structure to allow setting of additional compile time args such as APP_CPPFLAGS, etc that can be appended to the Makefile? This would allow us to pass in options without it having to be supported via the configure script



CSV for running tests:
format: shell_name, suite, vm_args, config_string, script_args

shell_name: [required] 
name of the shell, avmshell, avmshell_sd, coverage file to be used will be based on this value <shell_name>.cov

suite: [required] 
testsuite to invoke, list of current known suites listed below

vm_args: [optional] 
arguments passed into AVM such as -Dinterp, -Ojit etc

config_string: [optional] 
used by 'acceptance' suite to specify the config string if the runtests script is unable to determine the configuration

script_args: [optional] 
additional args to pass to the testsuite script 

Current testing suites are:
acceptance:
    cd test/acceptance; ./runtests.py -a <AVM> --vmargs=<vm_args> -c <config_string> <script_args>

performance:
    cd test/performance; ./runtests.py -a <AVM> --vmargs=<vm_args> <script_args>

selftests:
    <AVM> -Dselftest <vm_args???>

command-line:
    cd test/cmdline; ./runtests.py 

Open Issues:
[1] Will probably need to generate *-generic scripts for selftests and commandline suites as I do not believe that they currently exist.

Comment 2

8 years ago
re: [1] - Should be easy to add support into the compile-generic script to pick up extra configure_args env vars.
re [2] - I would rather we make changes to the configure script to expose anything additional that we need - even if that is a generic --extra-makefile-args arg

Do we need to have some way to control what runs on each platform?  If we are running on arm-linux I would think that we might need to pare down the testrun due to time limitations. 

re (second) [1] - we do have a run-selftest-generic.sh script already.   I don't think we have one for the commandline suites
(Assignee)

Comment 3

8 years ago
(In reply to comment #2)
> re: [1] - Should be easy to add support into the compile-generic script to pick
> up extra configure_args env vars.

Good idea, that had not crossed my mind but is a perfect way to implement.

> re [2] - I would rather we make changes to the configure script to expose
> anything additional that we need - even if that is a generic
> --extra-makefile-args arg
> 

Logged as bug# 592709

> Do we need to have some way to control what runs on each platform?  If we are
> running on arm-linux I would think that we might need to pare down the testrun
> due to time limitations. 
> 

Only thing that I can think of is limiting the amount of tests that are run by setting up a "codecoverage" configuration in the testconfig

Comment 4

8 years ago
re [1] compiling: If I understand this correctly we don't need an additional
mechanism to pass '--mac-sdk' or '--target'. They are parsed by getopt.py in
the same manner as '--enable-shell' and '--enable-debug' so they can just be
part of the configuration options list in the csv that gets passed to
compile-generic.sh. If we do need to add some other '--extra-makefile-args'
arg, as Chris mentions in [2] in Comment 2, it's easy to add parsing of that to
getopt.py for later processing in configure.py.

re [2]: It would be easy to have configure.py search for env vars like
'APP_CPPFLAGS' (or whatever we want to call it) and add their contents to
APP_CPPFLAGS (or other flag lists) in the Makefile. The only question would be
at what point we want to do it.
(Assignee)

Comment 5

8 years ago
(In reply to comment #4)
> re [1] compiling: If I understand this correctly we don't need an additional
> mechanism to pass '--mac-sdk' or '--target'. They are parsed by getopt.py in
> the same manner as '--enable-shell' and '--enable-debug' so they can just be
> part of the configuration options list in the csv that gets passed to
> compile-generic.sh. 

Issue with having --target in the compile.csv file is that the value is unique to the platform that it is compiling on and is not generic across windows, linux and mac.

windows: --target=x86_64-windows
mac: --target=x86_64-darwin
linux: --target=x86_64-linux

--mac-sdk is really only valid on mac and does not make sense to be specified when building on windows.

My proposed solution to this is to have a platform specific file that not only sets the necessary environment vars but will also contain an additional entry that contains platform specific options to pass to configure.py. 

This all only applies to the code coverage script and is not something that configure.py would ever know about.
(Assignee)

Comment 6

8 years ago
Created attachment 472652 [details] [diff] [review]
Initial patch for generic code-coverage

This patch will:

1) compile a code coverage instrumented shell, using the configure options listed in the compile.csv file. The format of the csv file for compiling is:
   display_name, shell_name, configure options, env_script, notes
This is the same format listed in comment #1

The env_script can also contain "CONFIGURE_ARGS" which are appended to the configure.py options in the compile-generic.sh script

2) run tests using an instrumented shell, this is driven by the test.csv file. The format of the csv is:
    shell_name, suite, vm_args, config_string, script_args
This is the same format listed in comment #1    

3) combine all of the generated *.cov files into a single code coverage file.
Attachment #472652 - Flags: review?(dschaffe)
(Assignee)

Comment 7

8 years ago
Comment on attachment 472652 [details] [diff] [review]
Initial patch for generic code-coverage

I have tested this script on mac, windows and linux.

The linux machine that we have for deep testing is not able to run a 64bit os, so for now we will have to generate a second set of compile/test csv files that only contain 32bit entries (until we can get a new machine).

The contents of the csv files is only meant to be a starting point and not the definitive list of things that we will compile and test.
Attachment #472652 - Flags: review?(jsudduth)

Comment 8

8 years ago
why are we using real hardware for linux deep testing instead of a virtual machine?
(Assignee)

Comment 9

8 years ago
(In reply to comment #8)
> why are we using real hardware for linux deep testing instead of a virtual
> machine?

This is real hardware, running on an old Pentium M machine. Hopefully the virtualization effort in the lab will kick in as this machine is on the list to be virtualized.

Comment 10

8 years ago
Comment on attachment 472652 [details] [diff] [review]
Initial patch for generic code-coverage

in zerowing using {table-plus}{csv...} the # is not known as a comment field so it displays in the table.

In the first line describing the fields you could omit the # and when parsing assume the 1st line is a description and skip it.  Also for the same reason the lines # AIR Config and # Only enable system-malloc should be moved to the notes column.  

patch looks good +
Attachment #472652 - Flags: review?(dschaffe) → review+

Comment 11

8 years ago
Comment on attachment 472652 [details] [diff] [review]
Initial patch for generic code-coverage

Looks good. Spotted a small typo around line 318 ("selfttest").
Attachment #472652 - Flags: review?(jsudduth) → review+
(Assignee)

Comment 12

8 years ago
Created attachment 473054 [details] [diff] [review]
version 2 of generic code coverage

Looking for a quick review of the following areas, otherwise this is the same patch but with old code-coverage files being removed:

- buildbot configuration
   - compiling and testing is handled via the deep_codecoverage buildstep
   - combining coverage results for a single platform and all platforms is handled via deep_codecoverage_process (calls the same script as before codecoverage-process.sh, just that it is moved into the 'all' dir)

- updated compile and test csv files to match what is currently being done, I will post a follow-up patch to increase the coverage once we can match the coverage results that are currently being produced

- updated the compile_shell() to abort the run if a compilation fails, this matches up with the way it used to work before (build-check), if a single shell fails to compile then abort running any coverage
Attachment #472652 - Attachment is obsolete: true
Attachment #473054 - Flags: review?(dschaffe)

Comment 13

8 years ago
Comment on attachment 473054 [details] [diff] [review]
version 2 of generic code coverage

+ reviewed patch 589168_v2.patch looks good
Attachment #473054 - Flags: review?(dschaffe) → review+
(Assignee)

Comment 14

8 years ago
Comment on attachment 473054 [details] [diff] [review]
version 2 of generic code coverage

Patch pushed as 5170:e4dccd893941
(Assignee)

Comment 15

8 years ago
Created attachment 473135 [details] [diff] [review]
Fix the calling of the runner from buildbot

need to use "-b #" instead of "--buildnum=#"
Attachment #473135 - Flags: review?(dschaffe)

Updated

8 years ago
Attachment #473135 - Flags: review?(dschaffe) → review+
(Assignee)

Comment 16

8 years ago
Comment on attachment 473135 [details] [diff] [review]
Fix the calling of the runner from buildbot

Patch pushed as 5176:9c6aba33a25e
(Assignee)

Comment 17

8 years ago
New code coverage script is generating coverage results that are matching what was being generated before. Actually slightly better now that all platforms are running selftest, previously only mac was running selftest.

Closing this bug since script and framework is now in place and we can increase coverage via new compilation targets and testing via different bugs.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

8 years ago
latest combined coverage file is available @ http://tamarin-builds.mozilla.org/analytics/codecoverage/latest/
Status: RESOLVED → VERIFIED
(Assignee)

Updated

7 years ago
Blocks: 592509
Flags: flashplayer-bug-
You need to log in before you can comment on or make changes to this bug.