Closed Bug 1014328 Opened 10 years ago Closed 10 years ago

Add long-running steeplechase test for use in the QA Lab infrastructure

Categories

(Core :: WebRTC, defect)

Product:
Core
Component:
WebRTC
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla33

People

(Reporter: gmealer, Assigned: gmealer)

Details

Attachments

(1 file, 1 obsolete file)

1014328.patch
7.95 KB, patch
drno
: review+
Details | Diff | Splinter Review 
As part of our WebRTC QA work, we've put together a basic setup in the lab. 

We want a test that performs a basic audio/video connection between real devices for several hours and looks for network activity to be maintained during that time.
Attached patch 1014328.patch (obsolete) — — Splinter Review 
Test runs for 3 hours per the defaults on generateIntervalCommand(). Was left to defaults on purpose, since we may play with the time on this and the other long-runners we have in mind, and I wanted a SPOT.

long.js contains the support pieces for setting up a poll link in the command chain and processing the stats, should make it easy to clone the other candidate tests to long-runners.

Alternate steeplechase ini file is included, since we don't want this as part of the standard run.
Attachment #8426701 - Flags: review?(rjesup)
Attachment #8426701 - Flags: review?(drno)
One note: the "dump()" is on purpose for the stats output--we dump to stdout to be able to tail logs on the steeplechase-driven clients to see ongoing activity. Using info() didn't output correctly in that situation.

Also, the test is exceptionally chatty--way more so than if we intended it to be run as part of the standard execution. This is appropriate for our use in the QA Lab environment.
Comment on attachment 8426701 [details] [diff] [review]
1014328.patch

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

::: dom/media/tests/mochitest/steeplechase_long.ini
@@ +1,3 @@
> +[DEFAULT]
> +support-files =
> +  head.js

Do these get MPL headers?  I realize it's trivial

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideoCombined_long.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

MPL
Attachment #8426701 - Flags: review?(rjesup) → review+
Comment on attachment 8426701 [details] [diff] [review]
1014328.patch

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

Just NIT's, feel free to address them or ignore them for now.
Otherwise looks good to me.

::: dom/media/tests/mochitest/long.js
@@ +57,5 @@
> +  dump(outputStr);
> +}
> +
> +
> +var _lastStats = {};

I would have preferred to have this attached to the PeerConnection, to prevent this becoming a blocker in the future if we want to look at more then just one PeerConnection.

@@ +81,5 @@
> +  ];
> +
> +  if (_lastStats[label] === undefined) {
> +    _lastStats[label] = stats;
> +  } else {

NIT: as line 84 and 107 do exactly the same, the if condition could be changed to '!== undefined' and line 107 getting executed all the time outside of the if condition.

::: dom/media/tests/mochitest/steeplechase_long.ini
@@ +1,3 @@
> +[DEFAULT]
> +support-files =
> +  head.js

None of our current *.ini or *.html test files have MLP boiler plate statements. No reason to not add them. Just saying that we should fix them all if we decide to add them,

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideoCombined_long.html
@@ +24,5 @@
> +    options = options || {};
> +    options.commands = commandsPeerConnection.slice(0);
> +    options.commands.push(generateIntervalCommand(verifyConnectionStatus));
> +
> +    test = new PeerConnectionTest(options);

NIT: it might be easier to first create the PeerConnectionTest() and then modify it with the high level function of the chain, e.g. test.chain.insertAfter();
Attachment #8426701 - Flags: review?(drno) → review+
(In reply to Nils Ohlmeier [:drno] from comment #4)

> ::: dom/media/tests/mochitest/long.js
>
> > +var _lastStats = {};
> 
> I would have preferred to have this attached to the PeerConnection, to
> prevent this becoming a blocker in the future if we want to look at more
> then just one PeerConnection.

Makes sense. These handle more than one PeerConnection now, in the mochitest version of the execution. _lastStats is a dictionary of stats objects, keyed by the "label" of the stats block. 

In the current implementation, for example, the local peer connection only saves one instance of stats, with the label "local", as opposed to "remote". Each subsequent stats check overwrites the last. But you could have one PC save multiple stats, more than two PCs, etc.

I didn't want to hang it off the PeerConnection object because this was actually more flexible, and because I was trying hard not to change basic infrastructure (i.e. modify pc.js) according to the rather specialized needs of these tests.

> 
> @@ +81,5 @@
> > +  ];
> > +
> > +  if (_lastStats[label] === undefined) {
> > +    _lastStats[label] = stats;
> > +  } else {
> 
> NIT: as line 84 and 107 do exactly the same, the if condition could be
> changed to '!== undefined' and line 107 getting executed all the time
> outside of the if condition.

Good point, I'll make this change.

> 
> ::: dom/media/tests/mochitest/steeplechase_long.ini
> @@ +1,3 @@
> > +[DEFAULT]
> > +support-files =
> > +  head.js
> 
> None of our current *.ini or *.html test files have MLP boiler plate
> statements. No reason to not add them. Just saying that we should fix them
> all if we decide to add them,

Guessing we typically don't do it for .ini, since http://www.mozilla.org/MPL/headers/ doesn't include ;commented versions. I'll add it for HTML in this one.

> NIT: it might be easier to first create the PeerConnectionTest() and then
> modify it with the high level function of the chain, e.g.
> test.chain.insertAfter();

Would be messy. The PeerConnectionTest constructor modifies the chain by adding its own teardown when you instantiate. So I can't just append, and would have to insertAfter*(). That means now I have to know the links of the chain to tell it what id to use, and I shouldn't have to know that.

  // Insert network teardown after testcase execution.
  if (netTeardownCommand) {
    this.chain.append(netTeardownCommand);
  }

Since in general PCT makes zero guarantees of what the default chain is (so I'd have to know that too) or what it'll do with it in construction, I really don't like counting on it. My take is if you want a specific chain best to create it and pass it in verbatim. 

But second test we do, we probably should move this into a helper function in long.js.

I'll make updates and resubmit for a brief look.
Attached patch 1014328.patch — — Splinter Review 
Made the change for the redundant if tree, added the MPL header to html. 

Also caught and fixed errors where I was inadvertently using global variables for for..in loops.

Ran against the QA lab with no problems. Note that these changes will not affect the main steeplechase or mochitest test run. Only the custom .ini file references the changed files, and it's only used in the QA lab.

Since I let this go for a bit, looking for a quick r+ again before we request landing.
Attachment #8426701 - Attachment is obsolete: true
Attachment #8445604 - Flags: review?(drno)
> Made the change for the redundant if tree, added the MPL header to html. 

...and also added turnConfig.js to the custom .ini and the html file.
Comment on attachment 8445604 [details] [diff] [review]
1014328.patch

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

Looks good to me
Attachment #8445604 - Flags: review?(drno) → review+
Requesting checkin. 

As mentioned in comment 6, the changes aren't in any files referenced by the mochitest or "official" steeplechase manifests. They're only relevant to the QA lab's run, which has been tried.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ff5d3c1b9856
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: