Closed Bug 1337525 Opened 7 years ago Closed 7 years ago

Add mochitests for WebRtc inbound-rtp stats

Categories

(Core :: WebRTC, defect, P2)

Product:
Core
Component:
WebRTC
Platform:
All
Unspecified
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: ng, Assigned: ng)

References

(Blocks 3 open bugs)

Details

Attachments

(1 file)

Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;
59 bytes, text/x-review-board-request
jib
: review+
Details 
Many changes are in the pipe for stats, both in the spec and from webrtc.org. It is easy to unintentionally break stats as there is not much test coverage.
Rank: 20
Priority: -- → P2
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review112174

You've been busy! TBH I expected something more pedantic and less thorough, something perhaps more like user-code examining stats one by one. There's more testing of types here than I usually see in JS tests. Others may disagree, but I find types matter less in JS (we trust WebIDL). I would focus more on testing that values make sense and are present than what their types are.

So I think there's room to simplify here, if you agree. I'm a bit concerned about maintainability, as it took me a while to wrap my head around the structure. Are we confident the array structure is a net win after we simplify, over e.g. a more pedantic giant for-loop over the returned data testing each stat with various helpers? If you are then great. I just ask as I see there are more stats yet to add. I also had some ideas.

Style nits and comments follow, so bear with me. r- for a couple of bugs.

::: dom/media/tests/mochitest/mochitest.ini:275
(Diff revision 3)
>  [test_peerConnection_remoteReofferRollback.html]
>  skip-if = (android_version == '18') # android(Bug 1189784, timeouts on 4.3 emulator)
>  [test_selftest.html]
>  # Bug 1227781: Crash with bogus TURN server.
>  [test_peerConnection_bug1227781.html]
> +[test_peerConnection_webrtcStats.html]

Maybe test_peerConnection_stats.html? The peerConnection part implies webrtc.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:2
(Diff revision 3)
> +<!DOCTYPE HTML>
> +<!-- This should be expanded to include DataChannels -->

DataChannels is future work rather than an omission, so no need for comment imho. If you leave it, add a bug #.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:11
(Diff revision 3)
> +</head>
> +<body>
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    bug: "TODO",

bug #

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:15
(Diff revision 3)
> +  // Wrap ok so that it returns the value of its first parameter
> +  const r_ok = (retVal, ...params) => {
> +    ok(retVal, ...params);
> +    return retVal;
> +  }

I'm not sold on this helper. r_ok appears in 13 places, and only a subset of those places seem to rely on the functionality it adds. I found it harder to read when I was wondering whether the return value mattered.

Also, a couple of those places could benefit from using is() instead of ok(), and we don't want r_is().

I think the code would be cleaner without it.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:16
(Diff revision 3)
> +    bug: "TODO",
> +    title: "webRtc Stats composition and sanity"
> +  });
> +
> +  // Wrap ok so that it returns the value of its first parameter
> +  const r_ok = (retVal, ...params) => {

s/const/var/ on arrow functions for consistency. Applies throughout.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:21
(Diff revision 3)
> +  // Predicate Generators
> +  //   Notice this stops processing as soon as a failing condition is met
> +  //   so one can rely on previous checks instead of having to re-check base
> +  //   assumptions, e.g. field existance, in each subtest.
> +  const PRED_AND = (...p) => {

In general, I wouldn't worry about fallout from a failing test. Is this is necessary other reasons than that?

Seems like dropping this requirement would simplify the testing logic a lot (then you wouldn't have to return every condition, saving a lot of JS, and maybe be able to refactor more easily).

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:25
(Diff revision 3)
> +
> +  // Predicate Generators
> +  //   Notice this stops processing as soon as a failing condition is met
> +  //   so one can rely on previous checks instead of having to re-check base
> +  //   assumptions, e.g. field existance, in each subtest.
> +  const PRED_AND = (...p) => {

var predAnd =

"In JavaScript, functions should use camelCase but should not capitalize the first letter." [1]

(The PC_LOCAL_TEST_* names are an exception, largely for historical reasons, as they appear in output).

Applies throughout.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:26
(Diff revision 3)
> +  // Predicate Generators
> +  //   Notice this stops processing as soon as a failing condition is met
> +  //   so one can rely on previous checks instead of having to re-check base
> +  //   assumptions, e.g. field existance, in each subtest.
> +  const PRED_AND = (...p) => {
> +    return data => -1 == p.find(test => !test(data));

[].find() returns undefined, not -1. Try [].some().

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:42
(Diff revision 3)
> +  //    fieldName: theNameOfTheField,
> +  //    statsObj: theContainingStatsObject,
> +  //    statsReport: theReportContainingTheStatsObject,
> +  //  }
> +
> +  const TYPE_DOMSTRING = (data) => {

Redundant () around single arrow function argument. Applies throughout.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:43
(Diff revision 3)
> +    var res = typeof(data.value) == "string";
> +    return r_ok(res, data.fieldName + " is of type DOMSTRING");

Usually for comparisons we prefer using:

    is(typeof(data.value), "string", "...");

That way the value is logged if wrong. Applies throughout.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:53
(Diff revision 3)
> +  const VALUE_NOT_EMPTY = (data) => {
> +    var res = true;
> +    if( ['Undefined','null'].indexOf(typeof(data.value)) != -1 ){
> +      res = false;
> +    }
> +    if (typeof(data.value) == 'string') {
> +      res = data.value.length > 0;
> +    }
> +    ok(res, data.fieldName + " is not EMPTY");
> +    return res;
> +  }

Isn't this whole function just:

    data => ok(data, "...");

?

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:55
(Diff revision 3)
> +    return res;
> +  }
> +
> +  const VALUE_NOT_EMPTY = (data) => {
> +    var res = true;
> +    if( ['Undefined','null'].indexOf(typeof(data.value)) != -1 ){

s/if( /if (/

Also, have a look at [].includes().

But more importantly, typeof(null) == "object".

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:58
(Diff revision 3)
> +  const VALUE_NOT_EMPTY = (data) => {
> +    var res = true;
> +    if( ['Undefined','null'].indexOf(typeof(data.value)) != -1 ){
> +      res = false;
> +    }
> +    if (typeof(data.value) == 'string') {

We should stick to double-quotes throughout for consistency (or single-quotes, just pick one).

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:65
(Diff revision 3)
> +  const TYPE_NONNEGATIVE_NUMBER = (data) => {
> +    var res = typeof(data.value) == "number" && data.value >= 0;
> +    ok(res, data.fieldName + " with value " + data.value + " is a nonnegative number");
> +    return res;
> +  }
> +
> +  const TYPE_NONNEGATIVE_INTEGER = (data) => {
> +    var res = Number.isInteger(data.value) && data.value >= 0;
> +    ok(res, data.fieldName + " with value " + data.value + " is a nonnegative integer");
> +    return res;
> +  }

Can these two be combined?

Since we're effectively testing a c++ implementation, WebIDL enforces types for us.

Granted, this is less true for getStats due to unfortunate use of Object in its definition, but I would still be surprised to e.g. see a double where a long was expected.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:65
(Diff revision 3)
> +    }
> +    ok(res, data.fieldName + " is not EMPTY");
> +    return res;
> +  }
> +
> +  const TYPE_NONNEGATIVE_NUMBER = (data) => {

s/TYPE_NONNEGATIVE_NUMBER/isNonNegativeNumber/ ?

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:77
(Diff revision 3)
> +  const VALUE_IS_VALID_SSRC = (data) => {
> +    if(!data.value) return false;
> +    var number = data.value;

You could use destructuring here. e.g.:

    var valueIsValidSsrc = ({value}) => {
      if (!value) return false;

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:78
(Diff revision 3)
> +    ok(res, data.fieldName + " with value " + data.value + " is a nonnegative integer");
> +    return res;
> +  }
> +
> +  const VALUE_IS_VALID_SSRC = (data) => {
> +    if(!data.value) return false;

s/if(/if (/

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:81
(Diff revision 3)
> +    // If the SSRC is stored as a string parse it
> +    if (typeof(number) == 'string') {

ssrc is always a string AFAIK. When we change it to a number, we change the test.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:82
(Diff revision 3)
> +    if (typeof(number) == 'string') {
> +      // Make sure we only take numbers in decimal without exponentiation.
> +      // parseInt will best effort parse from the start of the string, stopping when it
> +      // hits an unrecognized character, e.g. parseInt("54 ok") returns 54.
> +      if (-1 != [...number].find(c => -1 != '0123456789'.indexOf(c))) {
> +        number = Number.parseInt(number, 10);

This is a lot more thorough than I'd expect. E.g. I think I'd take ok(value > 0, "...")

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:86
(Diff revision 3)
> +    // If the SSRC is stored as a string parse it
> +    if (typeof(number) == 'string') {
> +      // Make sure we only take numbers in decimal without exponentiation.
> +      // parseInt will best effort parse from the start of the string, stopping when it
> +      // hits an unrecognized character, e.g. parseInt("54 ok") returns 54.
> +      if (-1 != [...number].find(c => -1 != '0123456789'.indexOf(c))) {

find does not return -1.

I'm also not a fan of the reverse ordering in comparisons. Can we keep the expression being tested on the left for readability and consistency with other tests? Applies throughout.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:98
(Diff revision 3)
> +    res = typeof(number) == 'number';
> +    // Perhaps a warning when 0 or 1, as they are suspicious
> +    // but valid.
> +    res = res && number >= 0;
> +    res = res && Number.isInteger(number);
> +    r_ok(res && number < Math.pow(2,32), data.fieldName + " with value " + data.value + " is a valid SSRC");

We can use 2 ** 32 now.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:110
(Diff revision 3)
> +  //  }
> +
> +  //
> +  // Contains a mapping of stats types to tests
> +  //
> +  const EXPECTED_FIELDS_BY_TYPE = {

s/EXPECTED_FIELDS_BY_TYPE/expectedFieldsByType/ [2]

Also, for clarity, we tend to reserve const for non-object constants. e.g. enums. The reason is const only applies to the object reference, and doesn't freeze its contents.

Applies throughout.

[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_objects

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:111
(Diff revision 3)
> +
> +  //
> +  // Contains a mapping of stats types to tests
> +  //
> +  const EXPECTED_FIELDS_BY_TYPE = {
> +    'inbound-rtp' : {

No quotes around literal object keys, and no space before colon [2]. Applies throughout.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:111
(Diff revision 3)
> +    'inbound-rtp' : {
> +        'expected' : {

Indent remainder of this object by 2 less.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:124
(Diff revision 3)
> +          'timestamp' : {
> +            tests:[TYPE_NONNEGATIVE_NUMBER]
> +          },

Does this object ever include keys other than tests? If not, then why not remove one level of nesting? E.g.

    timestamp: [typeNonNegativeNumber],

Should condense a lot better. {skip} could be []. Do we need {bugs}?

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:238
(Diff revision 3)
> +          'gapLossRate' : {bugs:["TODO"]},
> +        },
> +        'deprecated' : {
> +          'discardedPackets' : {bug:["TODO"]},

bug or bugs?

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:270
(Diff revision 3)
> +      'remote-candidate': {skip: true},
> +      'certificate': {skip: true},
> +  };
> +
> +  var CHECK_EXPECTED_FIELDS = stats => {
> +    var checked = Object.keys(EXPECTED_FIELDS_BY_TYPE).reduce( (a,b) => {

s/( (a,b)/((a, b)/

Applies throughout.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:271
(Diff revision 3)
> +      'certificate': {skip: true},
> +  };
> +
> +  var CHECK_EXPECTED_FIELDS = stats => {
> +    var checked = Object.keys(EXPECTED_FIELDS_BY_TYPE).reduce( (a,b) => {
> +        a[b] = {checked:false};

s/{checked:false}/{ checked: false }/

according to our style guide [2] (in spite of my personal preference).

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:317
(Diff revision 3)
> +        if (optional in stat && 'tests' in testPlan) {
> +          testPlan.tests.forEach( test => {
> +            test({
> +              value:stat[optional],
> +              fieldName:optional,
> +              statsObj:stat.type,

Is type a stats object? Also, space after colon.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:341
(Diff revision 3)
> +      var allKnownFields = new Array(...Object.keys(expectations['expected']),
> +                                     ...Object.keys(expectations['optional']),
> +                                     ...Object.keys(expectations['deprecated']));

Instead of new Array() just do []

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:355
(Diff revision 3)
> +      ok('tests' in expectations, stat.type + ' has test plan.');
> +
> +      //
> +      // Perform test plan over the whole stats type
> +      //
> +      if ('tests' in expectations) {

Any reason to prefer this over:

    if (expectations.tests) {

?

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:356
(Diff revision 3)
> +        var testPlan = expectations.tests;
> +        testPlan.forEach( test =>{
> +          test({
> +            statsObj: stat,
> +            statsReport: stats,
> +          });
> +        });

Just a thought from seeing this in a couple of places. Why not rename statsObj to stat and statsReport to stats?


Then the above becomes a one-liner:

    expectations.tests.forEach(test => test({ stat, stats }));

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:372
(Diff revision 3)
> +    });
> +    // Let the test decide if the coverage was adequate.
> +    return checked;
> +  }
> +
> +  var PC_LOCAL_TEST_LOCAL_STATS = test => {

Our convention is to use function declarations for PC_* tests. e.g.

    function PC_LOCAL_TEST_LOCAL_STATS(test) {

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:373
(Diff revision 3)
> +    // Let the test decide if the coverage was adequate.
> +    return checked;
> +  }
> +
> +  var PC_LOCAL_TEST_LOCAL_STATS = test => {
> +    test.pcLocal.getStats().then(stats => {

Return the promise.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:380
(Diff revision 3)
> +      CHECK_EXPECTED_FIELDS(stats);
> +    });
> +  }
> +
> +  var PC_REMOTE_TEST_REMOTE_STATS = test => {
> +    test.pcRemote.getStats().then(stats => {

Return the promise.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats.html:390
(Diff revision 3)
> +    test.chain.insertAfter('PC_LOCAL_WAIT_FOR_MEDIA_FLOW', [PC_LOCAL_TEST_LOCAL_STATS]);
> +
> +    test.chain.insertAfter('PC_REMOTE_WAIT_FOR_MEDIA_FLOW', [PC_REMOTE_TEST_REMOTE_STATS]);

Maybe inline PC_LOCAL_TEST_LOCAL_STATS and PC_REMOTE_TEST_REMOTE_STATS?
Attachment #8834625 - Flags: review?(jib) → review-
Assignee: nobody → na-g
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review114206

I like this direction!

Lots of little comments, and I'm probably being greedy here, but I'd love it if we could somehow evolve the "pedantic" part to rely on a controlled setup with a bit of time delay to provide us with more invariants to measure against, e.g. so we can start to expect > 0 values, and other assumptions (see comments). LMKWYT.

Also, see my proxy idea. I stopped 2/3rds through the individual tests as there's probably enough to look at for another round.

It's great to finally get this done!

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:12
(Diff revision 5)
> +    title: "webRtc Stats composition and sanity"
> +  });
> +var statsExpectedByType = {
> +  'inbound-rtp': {

FYI:

"Double-quoted strings (e.g. "foo") are preferred to single-quoted strings (e.g. 'foo') in JavaScript" - [1]

Disclaimer: From a grep of our tests, I see I'm an offender myself on this one. Take that as you will.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Literals

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:16
(Diff revision 5)
> +      expected: ['id', 'timestamp', 'type', 'ssrc', 'isRemote', 'mediaType', 'packetsReceived', 'bytesReceived',
> +          'jitter',],
> +      optional: ['remoteId', 'mozRtt', 'bitrateMean', 'bitrateStdDev', 'framerateMean', 'framerateStdDev', 'discardedPackets', 'packetsDiscarded', 'packetsLost',],
> +      unimplemented: ['mediaTrackId', 'transportId', 'codecId','framesDecoded',
> +          'packetsDiscarded', 'associateStatsId', 'firCount', 'pliCount', 'nackCount', 'sliCount', 'qpSum', 'packetsRepaired',
> +          'fractionLost', 'burstPacketsLost', 'burstLossCount', 'burstDiscardCount', 'gapDiscardRate', 'gapLossRate',],

Wordwrap to 80

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:18
(Diff revision 5)
> +  });
> +var statsExpectedByType = {
> +  'inbound-rtp': {
> +      expected: ['id', 'timestamp', 'type', 'ssrc', 'isRemote', 'mediaType', 'packetsReceived', 'bytesReceived',
> +          'jitter',],
> +      optional: ['remoteId', 'mozRtt', 'bitrateMean', 'bitrateStdDev', 'framerateMean', 'framerateStdDev', 'discardedPackets', 'packetsDiscarded', 'packetsLost',],

remoteId is not optional. When isRemote = true, remoteId points back to the isRemote = false dictionary (why it was renamed associateStatsId) [1]

Rather than expected vs. optional, I think the test should break expectations up into four categories:

  audio inbound-rtp  (RTCRtpInboundRtpStats)
  audio inbound-rtcp (RTCRtpOutboundRtpStats)
  video inbound-rtp  (RTCRtpInboundRtpStats)
  video inbound-rtcp (in an RTCRtpOutboundRtpStats)

And same for outbound, so 8. I could be wrong, but  this way I hope no stats end up being truly optional.

[1] https://w3c.github.io/webrtc-stats/#example-of-association-of-local-and-remote-statistics

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:19
(Diff revision 5)
> +var statsExpectedByType = {
> +  'inbound-rtp': {
> +      expected: ['id', 'timestamp', 'type', 'ssrc', 'isRemote', 'mediaType', 'packetsReceived', 'bytesReceived',
> +          'jitter',],
> +      optional: ['remoteId', 'mozRtt', 'bitrateMean', 'bitrateStdDev', 'framerateMean', 'framerateStdDev', 'discardedPackets', 'packetsDiscarded', 'packetsLost',],
> +      unimplemented: ['mediaTrackId', 'transportId', 'codecId','framesDecoded',

missing space after one comma

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:45
(Diff revision 5)
> +var checkExpectedFields = report => {
> +  report.forEach( stat => {

No space after ( .

Also, we could lose a level of indent here if we want (just an idea):

    var checkExpectedFields = report => report.forEach( stat => {
      ...
    };

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:47
(Diff revision 5)
> +//
> +//  Checks that the fields in a report conform to the expectations in statExpectedByType
> +//
> +var checkExpectedFields = report => {
> +  report.forEach( stat => {
> +    var expectations = statsExpectedByType[stat.type];

Use let for variables inside functions.

Applies throughout.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:57
(Diff revision 5)
> +    // Check that each field is not deprecated
> +    Object.keys(stat).forEach(field => {
> +      ok(!expectations.deprecated.some(f => f == field),
> +        'Expected stat field ' + stat.type + '.' + field + ' does not exist');
> +    });

I'd remove this. Testing for absense seems like low value, and there are no "deprecated" values in this test.

Also "deprecated" isn't the right word, since it usually means we still support it but with deprecation warnings.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:65
(Diff revision 5)
> +        'Expected stat field ' + stat.type + '.' + field + ' does not exist');
> +    });
> +    // Check that each field is either expected or optional
> +    var allowed = [...expectations.expected, ...expectations.optional];
> +    Object.keys(stat).forEach(field => {
> +      ok(allowed.some(f => f == field), 'Stat field ' + stat.type + '.' + field + ' is allowed');

Use

    allowed.includes(field)

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:70
(Diff revision 5)
> +      ok(allowed.some(f => f == field), 'Stat field ' + stat.type + '.' + field + ' is allowed');
> +    });
> +  });
> +}
> +
> +var pedanaticChecks = report => {

pedantic

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:76
(Diff revision 5)
> +    if (expectations.skip) {
> +      return;

We want to skip on !expectations as well I think (you do elsewhere).

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:82
(Diff revision 5)
> +      return;
> +    }
> +
> +    var tested = [];
> +
> +    if (stat.type == 'inbound-rtp') {

This will test both rtp and rtcp at once.

There are a couple of ways to skin this cat. When I enumerate stats, I typically test for !isRemote here, and instead follow remoteId as a sub-step to test that rtp and rtcp match (e.g. have ssrc's etc).

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:88
(Diff revision 5)
> +      //
> +      // Required fields
> +      //
> +
> +      // id
> +      ok(stat.id, stat.type + '.id exists');

Use

    is(stat.id, stat.type, "...");

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:89
(Diff revision 5)
> +      // Required fields
> +      //
> +
> +      // id
> +      ok(stat.id, stat.type + '.id exists');
> +      tested.push('id');

Normally I'd argue this is what reviews are for, but I agree there's an opportunity here to detect missing tests using info we already have. But I dislike the readability cost.

I think we can solve the cost using a proxy. Then there's no need to remember pushing every item.

See http://jsfiddle.net/jib1/3af43c21/

PS: Beware that if ("foo" in stat) won't work with proxies.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:91
(Diff revision 5)
> +
> +      // id
> +      ok(stat.id, stat.type + '.id exists');
> +      tested.push('id');
> +      // timestamp
> +      ok(stat.timestamp >= 0, stat.type + '.timestamp is not zero');

> 0

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:94
(Diff revision 5)
> +      tested.push('id');
> +      // timestamp
> +      ok(stat.timestamp >= 0, stat.type + '.timestamp is not zero');
> +      tested.push('timestamp');
> +      // type
> +      ok(stat.type, "Field .type exists and is not empty.");

Redundant. We know stat.type == 'inbound-rtp' at this point.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:97
(Diff revision 5)
> +      tested.push('timestamp');
> +      // type
> +      ok(stat.type, "Field .type exists and is not empty.");
> +      tested.push('type');
> +      // isRemote
> +      ok('isRemote' in stat, stat.type + '.isRemote exists');

We should follow remoteId here imho and test that it points to a valid record that matches ours (same ssrc).

Also, this may be an intermittent if we're not careful to wait a sufficient amount of time to ensure we get and RTCP packet. We may want to add a delay. Let's see on try.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:103
(Diff revision 5)
> +      ok(['audio', 'video'].some(v => v == stat.mediaType),
> +        stat.type + ".mediaType is 'audio' or 'video'");

Ads mentioned earlier, I'd test for video vs audio up top, as there are a lot of video-only stats (at least in the spec).

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:107
(Diff revision 5)
> +      // mediaType
> +      ok(['audio', 'video'].some(v => v == stat.mediaType),
> +        stat.type + ".mediaType is 'audio' or 'video'");
> +      tested.push('mediaType');
> +      // packetsReceived
> +      ok(stat.packetsReceived < 10000,

I think we should add a delay and test for > 0 as well if we can.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:112
(Diff revision 5)
> +      ok(stat.packetsReceived < 10000,
> +        stat.type + ".packetsReceived is a sane number for a short test. value="
> +        + stat.packetsReceived);
> +      tested.push('packetsReceived');
> +      // bytesReceived
> +      ok(stat.bytesReceived < Math.pow(10, 9), // Not a magic number, just a guess

Same here.

Also, I'm wondering since we control both sides of the connection, if we can add more assertions here, e.g. that the number of bytes received by the receiver are within a margin of error of the number of bytes sent.

Doing this would requires we set these assumptions up ahead of time.

::: dom/media/tests/mochitest/test_peerConnection_webrtcStats_pedantic.html:154
(Diff revision 5)
> +          + ".remoteId has a corresponding object in the report. value=" + stat.remoteId);
> +      }
> +      tested.push('remoteId');
> +
> +      // mozRtt
> +      if ('mozRtt' in stat) {

Would need to be changed to

    if (stat.mozRtt !== undefined) {

to be counted by a proxy.
Attachment #8834625 - Flags: review?(jib) → review-
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review114206

> Wordwrap to 80

Oh, noooooooo!

> I'd remove this. Testing for absense seems like low value, and there are no "deprecated" values in this test.
> 
> Also "deprecated" isn't the right word, since it usually means we still support it but with deprecation warnings.

I am removing this, but I will be resurrecting it when we have fields that are deprecated as you describe above to check that they are non-enumerable but still accessable via direct access.

> pedantic

I am not going to admit how many seconds I stared at this before I saw it.

> We want to skip on !expectations as well I think (you do elsewhere).

Now checks either !expectations || expectations.skip

> This will test both rtp and rtcp at once.
> 
> There are a couple of ways to skin this cat. When I enumerate stats, I typically test for !isRemote here, and instead follow remoteId as a sub-step to test that rtp and rtcp match (e.g. have ssrc's etc).

Ok. This was in the old structure but got lost in the shuffle.

> Use
> 
>     is(stat.id, stat.type, "...");

I don't follow.

> Normally I'd argue this is what reviews are for, but I agree there's an opportunity here to detect missing tests using info we already have. But I dislike the readability cost.
> 
> I think we can solve the cost using a proxy. Then there's no need to remember pushing every item.
> 
> See http://jsfiddle.net/jib1/3af43c21/
> 
> PS: Beware that if ("foo" in stat) won't work with proxies.

This won't work because the tests can reference fields not under the current test. The bloat associated with repeating ones self for each field was a key motivation behind the original structure. OTOH, tests probably shouldn't be too clever.

I could do something like:
var test = (field, fn)=>{
  fn();
  tested.push(field)
}

...

test("foo", =>{
  //This is my test
  ok( blah blah blah);
});

Which is not a space savings, but it does lexically scope what is under test.

> Redundant. We know stat.type == 'inbound-rtp' at this point.

This is staying so I don't have to make an exception to the test logic.

> We should follow remoteId here imho and test that it points to a valid record that matches ours (same ssrc).
> 
> Also, this may be an intermittent if we're not careful to wait a sufficient amount of time to ensure we get and RTCP packet. We may want to add a delay. Let's see on try.

The test waits for RTCP before executing.

> Ads mentioned earlier, I'd test for video vs audio up top, as there are a lot of video-only stats (at least in the spec).

I want to keep the structure flat, i.e. I don't want to create a 2x2 matrix of (remote vs. local) x (video vs. audio). I am testing isRemote and mediaType explicitly for each of them. This is not a general problem for other types of stats.

> I think we should add a delay and test for > 0 as well if we can.

We wait for RTCP, so no additional wait should be required.
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review112174

> s/{checked:false}/{ checked: false }/
> 
> according to our style guide [2] (in spite of my personal preference).

I don't know why I feel so strongly that the spaces are terrible, but I do ...

> Any reason to prefer this over:
> 
>     if (expectations.tests) {
> 
> ?

To avoid testing for truthiness.

> Just a thought from seeing this in a couple of places. Why not rename statsObj to stat and statsReport to stats?
> 
> 
> Then the above becomes a one-liner:
> 
>     expectations.tests.forEach(test => test({ stat, stats }));

Learned some new JS here.
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review114206

> I don't follow.

My mistake. I read it wrong.

> We wait for RTCP, so no additional wait should be required.

That might not be sufficient. We discussed leaving this for a follow-on "timelapse" test.
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review114206

> That might not be sufficient. We discussed leaving this for a follow-on "timelapse" test.

Agreed. I am checking for =< 0 in the latest revision.
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review115968
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review112174

> I don't know why I feel so strongly that the spaces are terrible, but I do ...

I agree with you. I myself use the compact form in fiddles. But I use spaces in code I commit to mozilla.

> To avoid testing for truthiness.

We're testing whether tests are present here. The Mozilla coding style is to embrace truthy and falsy for this. [1] [2]

This if-statement encapsulates code that uses expectations.tests as an array, allowing tests to be omitted without error. Our convention is to use truthy/falsy for this. We specifically don't care whether absence is achieved through {}, { tests: undefined }, { tests: false } or even { tests: [] }. That's our norm.

The "in" operator over-selects on falsy, which is unconventional except where we explicitly care about the difference between {}, { tests: undefined }, { tests: false }, and we don't here. As a reader of this code, the "in" operator made me stop to consider whether this code relied on this significance, when it doesn't. We want to reserve that for cases where it's justified/needed.

[1] "In JavaScript, == is preferred to ===." [1]
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

[2] "Do not compare x == true or x == false. Use (x) or (!x) instead."
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_practices
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review116026

I think we're getting close!  Arguably inbound and outbound are the most complicated stats, so once we get through these, the rest should be peanuts in comparison!

Misc nits, and I still have some concerns about overall structure. There's a lot of manual existence tests still, intertwined with the sanity tests in pedanticTests(). Their state logic is hard to follow, and they miss testing some cases (see comments). Ideally I'd love to see checkExpectedFields() handle all existence checks, and have that serve as an invariant for the subsequent pedanticTests(), which I think can be structured less like a list of discrete tests that way. See comments.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:14
(Diff revision 10)
> +  "inbound-rtp": {
> +      expected: ["id", "timestamp", "type", "ssrc", "isRemote", "mediaType",
> +        "packetsReceived", "bytesReceived", "jitter",],

Inconsistent indent.

I sometimes use 4 spaces for wrapped lines, but never after {

::: dom/media/tests/mochitest/test_peerConnection_stats.html:16
(Diff revision 10)
> +        "packetsReceived", "bytesReceived", "jitter",],
> +      optional: ["mozRtt", "bitrateMean", "bitrateStdDev",
> +        "framerateMean", "framerateStdDev", "discardedPackets",
> +       "packetsDiscarded", "packetsLost",
> +        "remoteId"], // Bug 1341421, remoteId is _not_ optional!

Inconsistent use indent and trailing commas.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:21
(Diff revision 10)
> +      unimplemented: ["mediaTrackId", "transportId", "codecId", "framesDecoded",
> +        "packetsDiscarded", "associateStatsId", "firCount", "pliCount",
> +        "nackCount", "sliCount", "qpSum", "packetsRepaired", "fractionLost",
> +        "burstPacketsLost", "burstLossCount", "burstDiscardCount",
> +        "gapDiscardRate", "gapLossRate",],

We should test that we're not implementing these. Otherwise this is dead code/data, and should be removed.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:79
(Diff revision 10)
> +    let tested = [];
> +    // Record what fields get tested.
> +    // To access a field foo without marking it as tested use stat.inner.foo
> +    let stat = new Proxy(statObj, {
> +      get(stat, key) {
> +        if(key == "inner") return stat;

if (

::: dom/media/tests/mochitest/test_peerConnection_stats.html:86
(Diff revision 10)
> +    ok(expectations, "Stat type " + stat.type
> +      + " has expectations. expectations=" + JSON.stringify(expectations));

Redundant (already in checkExpectedFields())

::: dom/media/tests/mochitest/test_peerConnection_stats.html:99
(Diff revision 10)
> +      //
> +      // Required fields
> +      //
> +
> +      // id
> +      ok(stat.id, stat.type + ".id exists");

We already know id exists from checkExpectedFields().

Instead, maybe check that stat.id == mapKey ?

where mapKey is from:

    report.forEach((statObj, mapKey) => {

::: dom/media/tests/mochitest/test_peerConnection_stats.html:102
(Diff revision 10)
> +
> +      // id
> +      ok(stat.id, stat.type + ".id exists");
> +
> +      // timestamp
> +      ok(stat.timestamp >= 0, stat.type + ".timestamp is not 0");

Fix test or string to make them agree.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:105
(Diff revision 10)
> +
> +      // timestamp
> +      ok(stat.timestamp >= 0, stat.type + ".timestamp is not 0");
> +
> +      // isRemote
> +      ok("isRemote" in stat, stat.type + ".isRemote exists");

Redundant. We already know isRemote exists from checkExpectedFields().

We also know it exists because it is invariant thanks to webidl (the binding code ensures it's always present):

https://dxr.mozilla.org/mozilla-central/rev/b06968288cff469814bf830aa90f1c84da490f61/dom/webidl/RTCStatsReport.webidl#32

::: dom/media/tests/mochitest/test_peerConnection_stats.html:107
(Diff revision 10)
> +        ok(stat.remoteId in report, "remoteId exists in report.");
> +        is(report[stat.remoteId].ssrc, stat.ssrc,
> +          "remote ssrc and local ssrc match.");

We should use the spec API. The legacy API is tested elsewhere.

    ok(report.has(stat.remoteId), "...");
    is(report.get(stat.remoteId).ssrc, stat.ssrc, "...");

Applies throughout.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:114
(Diff revision 10)
> +      // SSRC
> +      ok(stat.ssrc, stat.type + ".ssrc has a value");

I'd move this test above the isRemote one. That way if it ever fails, it's not preceeded by noise from the isRemote test which relies on ssrc and likely would fail too in that case.

Also, the single-word comments repeating what's tested on the next line, seem redundant to me, at least when the tests are trivial like this, but ymmv.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:139
(Diff revision 10)
> +      ok(stat.packetsLost < 100,
> +        stat.type + ".packetsLost is a sane number for a short test. value="
> +        + stat.packetsLost);
> +
> +      // jitter
> +      ok(stat.jitter < 10.00, // This should be much lower, Bug 1330575

Nit: Drop the .00

Also, we try to add TODO: in comments with bug #s

::: dom/media/tests/mochitest/test_peerConnection_stats.html:162
(Diff revision 10)
> +      if (stat.packetsDiscarded !== undefined) {
> +        ok(!stat.inner.isRemote,
> +          stat.type + ".packetsDiscarded is only set when isRemote is false");
> +      }

If I understand the spec correctly, we want to test that it is ALWAYS present for rtp, and NEVER present for rtcp. This doesn't do that. This pattern repeats for most of the remaining optional stats. 

Also, packetsDiscarded hasn't landed yet, and is listed under "unimplemented" above. So we should leave it to Bug 1335967 to add this, or if that bug lands before this one, fix it by removing the defensive if() statement around the test.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:172
(Diff revision 10)
> +      if (stat.remoteId) { // Remove `if` when Bug 1341421 is resolved
> +        ok(stat.remoteId !== undefined, stat.type + ".remoteId exists.");
> +      }

Redundant. Existence is already covered by checkExpectedFields()

::: dom/media/tests/mochitest/test_peerConnection_stats.html:181
(Diff revision 10)
> +      if (stat.inner.mediaType == 'audio' && stat.inner.type == 'inbound-rtp'
> +          && stat.inner.isRemote == false && !stat.inner.remoteId) {

!stat.inner.isRemote

Also, remove

    && !stat.inner.remoteId

to avoid silencing todo() (it's supposed to yell when things start working).

::: dom/media/tests/mochitest/test_peerConnection_stats.html:187
(Diff revision 10)
> +      if (stat.mozRtt !== undefined) {
> +        ok(stat.mozRtt >= 0, stat.type + ".mozRtt is sane.");
> +      }

If I read the spec right, mozRtt should always be present for isRemote == true, and never be present for isRemote == false. We should test that.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:209
(Diff revision 10)
> +      // check that the video only fields are set
> +      if (stat.inner.mediaType == "video" && !stat.inner.isRemote) {
> +        ok("framerateMean" in stat, stat.type
> +          + ".framerateMean is set when mediaType is 'video'" );
> +        ok("framerateStdDev" in stat,
> +          stat.type + ".framerateStdDev is set when mediaType is 'video'" );

We should also check that these are never present for audio.

One observation and idea:

It seems that testing for presence should be the job of checkExpectedFields()...

What if we broke up the "optional" category into "video", "audio" and "rtcp" ?

(We seem to have no rtcp stats that are video or audio only).

That would provide a nice invariant for the pedantic tests and we wouldn't have to these presence tests on a case by case basis, which can be tricky to get right.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:290
(Diff revision 10)
> +      // targetBitrate
> +      todo(stat.targetBitrate > 0,
> +        stat.type + ".targetBitrate is a sane. value="
> +        + stat.targetBitrate);
> +
> +      // roundTripTime
> +      todo(stat.roundTripTime > 0,
> +        stat.type + ".roundTripTime is not negative. value="
> +        + stat.roundTripTime);

This defeats the proxy. It seems premature to write tests for stats we haven't implemented yet.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:300
(Diff revision 10)
> +      // remote id
> +      if (stat.remoteId) { // Remove `if` when Bug 1341421 is resolved
> +        ok(stat.remoteId !== undefined, stat.type + ".remoteId exists.");
> +      }

Redundant.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:325
(Diff revision 10)
> +      // droppedFrames
> +      if (!stat.inner.isRemote && stat.inner.mediaType == "video") {
> +        ok(stat.droppedFrames >= 0,
> +          stat.type + ".droppedFrames is not negative. value="
> +          + stat.droppedFrames);
> +      }
> +      if (stat.droppedFrames !== undefined) {
> +        ok(!stat.inner.isRemote && stat.inner.mediaType == "video",
> +          stat.type + ".droppedFrames is only set when isRemote is false and "
> +          + "mediaType is 'video'");
> +      }

The above deals with a lot of state, and I find it hard to follow. This problem seems repeats for each stat tested.

pedanticChecks() still appears written as a series of discrete tests, instead of taking advantage of its imperative nature.

Assuming we can beef up checkExpectedFields() to deal with existence as mentioned earlier, I'd recommend the following overall structure instead:

    if (stat.type == "outbound-rtp") {
      // all common stats tests go here
      
      if (!stat.inner.isRemote) {
        // all rtcp-only stats tests go here
      } else {
        // common rtp-only stats here
        
        if (stat.inner.mediaType == "video") {
          // video-only stats tests go here
        } else {
          // audio-only stats tests go here
        }
      }
    }

Repeat for inbound. All other stats should hopefully be a lot simpler.
Attachment #8834625 - Flags: review?(jib) → review-
> if (!stat.inner.isRemote)

Make that: if (stat.inner.isRemote)
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review116026

> We should test that we're not implementing these. Otherwise this is dead code/data, and should be removed.

Good catch, another thing that didn't make the transition.

> !stat.inner.isRemote
> 
> Also, remove
> 
>     && !stat.inner.remoteId
> 
> to avoid silencing todo() (it's supposed to yell when things start working).

As it is written it isn't terribly useful, the problem is that it _may_ be empty, and todo will yell falsely. The string is poorly worded at least. I will rewrite this test.

> We should also check that these are never present for audio.
> 
> One observation and idea:
> 
> It seems that testing for presence should be the job of checkExpectedFields()...
> 
> What if we broke up the "optional" category into "video", "audio" and "rtcp" ?
> 
> (We seem to have no rtcp stats that are video or audio only).
> 
> That would provide a nice invariant for the pedantic tests and we wouldn't have to these presence tests on a case by case basis, which can be tricky to get right.

Does that make sense for other stats objects?
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review116026

> Does that make sense for other stats objects?

Other stats have no "optional" members AFAICT, "optional" being an artifact of reusing dictionaries among the RTCRtpStreamStats derivatives.
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review116942

This lgtm with some nits. I was expecting testing of non-existence to go with testing of existence. Not a requirement, just wanted to point it out, so you have options.

Except... I forgot to question the wait(10000) last time, which I think the automation guys will thank us for if we make smarter, so I'd like to see it again.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:17
(Diff revision 14)
> +    optional: ["mozRtt", "packetsLost", "remoteId",], // TODO:
> +      // Bug 1341421, remoteId is _not_ optional after remote stats have arrived

packetsLost should be in the expected pile here I think. Also, mozRtt is expected for rtcp, and wrong otherwise. How about:

    rtcpOnly: ["mozRtt"],
    optional: ["remoteId",]

?

::: dom/media/tests/mochitest/test_peerConnection_stats.html:30
(Diff revision 14)
> +    optional: ["remoteId",], // TODO: Bug 1341421, remoteId is _not_ optional
> +      // after remote stats have arrived

The resolution to bug 1341421 is likely that remoteId is either missing or a non-empty string.

So I think we're stuck with remoteId being optional, (unless we modify the test to guarantee rtcp packets, which I think we should save for a separate more controlled timelapse test), and we should remove this comment.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:48
(Diff revision 14)
> +["in", "out"].forEach(pre =>{
> +  with(statsExpectedByType[pre + "bound-rtp"]) {
> +      optional = [...optional, ...videoOnly];
> +  }

This allows videoOnly stats to appear outside of video, e.g. in audio, which I think is too soft a test. We should disallow videoOnly stats everywhere but video, not treat it as optional. - I see you've instead solved this in the pedantic test, but I half-expected this to be solved here. Seems cleaner to assert not just existence but non-existence here rather than to silence them here as optional, which seems structurally incorrect. Only remoteId so far is truly optional.

Also, don't use `with` [1]. It's forbidden in strict mode, and even though not all our tests use strict mode, the use of `with` is generally out of favor.

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/with

::: dom/media/tests/mochitest/test_peerConnection_stats.html:90
(Diff revision 14)
> +  });
> +});
> +
> +var pedanticChecks = report => {
> +  report.forEach((statObj, mapKey) => {
> +    let tested = [];

let tested = {};

::: dom/media/tests/mochitest/test_peerConnection_stats.html:117
(Diff revision 14)
> +    if (["inbound-rtp", "outbound-rtp"].includes(stat.type)) {
> +      //
> +      // Common RTCStreamStats fields
> +      //
> +
> +      // SSRC
> +      ok(stat.ssrc, stat.type + ".ssrc has a value");
> +
> +      // isRemote
> +      ok(stat.isRemote !== undefined, stat.type + ".isRemote exists.");
> +
> +      // mediaType
> +      ok(["audio", "video"].includes(stat.mediaType),
> +        stat.type + ".mediaType is 'audio' or 'video'");
> +
> +      // remote id
> +      // special exception, TODO: Bug 1341421
> +      if (stat.remoteId) {

This is good for the common parts. If we wanted to be extra paranoid, we could do a switch statement instead of if statements on type. I'm usually nervous about this pattern:

    if (condition) {
      do test;
    }

whenever there's no else-statement, because it's vulnerable to bitrot where tests silently disappear. Since we just did a test for existence earlier, it's probably fine here, though in tests I generally prefer to reduce codepaths. One way would be to use if-else, or even switch. E.g.:

    let doStreamTests = () => {...};

    switch(stats.type) {
      case "inbound-rtp":
        doStreamTests();
        //do inbound tests
        break;
      
      case "outbound-rtp":
        doStreamTests();
        //do outbound tests
        break;

      default:
        warn;
        break;
    }

::: dom/media/tests/mochitest/test_peerConnection_stats.html:133
(Diff revision 14)
> +      // special exception, TODO: Bug 1341421
> +      if (stat.remoteId) {
> +        ok(report.has(stat.remoteId), "remoteId exists in report.");

As mentioned, bug 1341421 won't remove the need for this if(), so the TODO should be removed.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:199
(Diff revision 14)
> +      if (!stat.inner.isRemote && stat.inner.mediaType == "video") {
> +        expectations.videoOnly.forEach(field => {

This if-statements's else clause contains the string " when isRemote is true", which does not match the condition. I suspect the string is wrong and the condition is right.

Whenever an else clause is tiny, and the main clause is not, I flip them around, making both visible without needing to scroll. E.g.:

    if (stat.inner.isRemote || stat.inner.mediaType != "video") {
      expectations.videoOnly.forEach(field => {
        ok(stat[field] === undefined, stat.type + " does not have field "
           + field + " for audio or when isRemote is true");
      });
    } else {
      // ...
    }

Applies to same code for "outbound-rtp" below as well.

I also half-expected this expectations part of the test to live in checkExpectedFields(), but that's fine.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:245
(Diff revision 14)
> +        expectations.videoOnly.forEach(field =>
> +        {
> +          ok(!stat[field] !== undefined, stat.type + " does not have field "

=== undefined

and move { to end of previous line.

Applies to same code for "outbound-rtp" below as well.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:253
(Diff revision 14)
> +            + field + " when isRemote is true");
> +        });
> +      }
> +    }
> +
> +    if (stat.type == "outbound-rtp") {

else if or switch?

::: dom/media/tests/mochitest/test_peerConnection_stats.html:268
(Diff revision 14)
> +      // remote id
> +      if (stat.remoteId) {
> +        ok(stat.remoteId in report, stat.type
> +          + ".remoteId has a corresponding object in the report. value="
> +          + stat.remoteId);
> +      }

Remove this no-op code that's also redundant since you already covered remoteId earlier.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:290
(Diff revision 14)
> +            + " when mediaType is video");
> +        });
> +
> +        // bitrateMean
> +        if (stat.bitrateMean !== undefined) {
> +          // TODO: uncomment when Bug 1341533 lands

Bug 1341533 will likely land first, as it seems ready, so don't forget to uncomment these before landing. Committing commented-out code is usually frowned on. I've reviewed the commented-out code FWIW.

What I usually do when I know this is the case, is mark this bug as blocking on bug 1341533, as a sign to reviewers what other patches are ahead of this one on the patch queue.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:344
(Diff revision 14)
> +    });
> +  });
> +}
> +
> +var PC_LOCAL_TEST_LOCAL_STATS = test => {
> +  return wait(10000).then(test.pcLocal.getStats().then(stats => {

(I didn't catch this last time, and sorry to add more work, but) For the sake of efficiency and automation resources, I think we should replace this `wait(10000)` with a 1-second polling of getStats until a remoteId is present, with a timeout of 10000.
Attachment #8834625 - Flags: review?(jib) → review-
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review116942

> This allows videoOnly stats to appear outside of video, e.g. in audio, which I think is too soft a test. We should disallow videoOnly stats everywhere but video, not treat it as optional. - I see you've instead solved this in the pedantic test, but I half-expected this to be solved here. Seems cleaner to assert not just existence but non-existence here rather than to silence them here as optional, which seems structurally incorrect. Only remoteId so far is truly optional.
> 
> Also, don't use `with` [1]. It's forbidden in strict mode, and even though not all our tests use strict mode, the use of `with` is generally out of favor.
> 
> [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/with

Optional stats are ones that do not appear in every stat object we expect to test. The video stats are why the optional category primarily exists. I have added a test to insure non-existance when mediaType is audio, which was the original intention.

> This is good for the common parts. If we wanted to be extra paranoid, we could do a switch statement instead of if statements on type. I'm usually nervous about this pattern:
> 
>     if (condition) {
>       do test;
>     }
> 
> whenever there's no else-statement, because it's vulnerable to bitrot where tests silently disappear. Since we just did a test for existence earlier, it's probably fine here, though in tests I generally prefer to reduce codepaths. One way would be to use if-else, or even switch. E.g.:
> 
>     let doStreamTests = () => {...};
> 
>     switch(stats.type) {
>       case "inbound-rtp":
>         doStreamTests();
>         //do inbound tests
>         break;
>       
>       case "outbound-rtp":
>         doStreamTests();
>         //do outbound tests
>         break;
> 
>       default:
>         warn;
>         break;
>     }

Going with 'else if' because of the length of the case bodies.

> Bug 1341533 will likely land first, as it seems ready, so don't forget to uncomment these before landing. Committing commented-out code is usually frowned on. I've reviewed the commented-out code FWIW.
> 
> What I usually do when I know this is the case, is mark this bug as blocking on bug 1341533, as a sign to reviewers what other patches are ahead of this one on the patch queue.

I plan is to get 1341533 landed after this and I am going to uncomment these tests in that patch.  If that sounds suboptimal to you, I am fine with what you said above, just ping me.

> (I didn't catch this last time, and sorry to add more work, but) For the sake of efficiency and automation resources, I think we should replace this `wait(10000)` with a 1-second polling of getStats until a remoteId is present, with a timeout of 10000.

I am going to push all the above changes minus this one so that you can review those earlier if you like. I am working on this now, and will push another update.
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review117722
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review116942

> packetsLost should be in the expected pile here I think. Also, mozRtt is expected for rtcp, and wrong otherwise. How about:
> 
>     rtcpOnly: ["mozRtt"],
>     optional: ["remoteId",]
> 
> ?

mozRtt is the only rtcpOnly field, I don't know that it needs its own category. It seems like it is adding more code without proportional benefit.

> I am going to push all the above changes minus this one so that you can review those earlier if you like. I am working on this now, and will push another update.

Should be fixed in the latest update.
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review118324

Existing code looks good. Newly added code needs revision though.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:201
(Diff revisions 14 - 16)
> +          }
> +          if (stat.inner.mediaType == "audio") {

I'd replace this if-statement with `} else {`.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:280
(Diff revisions 14 - 16)
> +          }
> +          if (stat.inner.mediaType == "audio") {

same here

::: dom/media/tests/mochitest/test_peerConnection_stats.html:343
(Diff revisions 14 - 16)
> +var waitForRtcp = pc => {
> +  return new Promise((resolve,reject) => {
> +    let detectRtcp = () => {
> +      pc.getStats().then(stats => {

Avoid the promise constructor anti-pattern [1].

In short, never use the promise constructor except to wrap legacy callback APIs. In other words, in an ideal world where promise-adoption was 100%, the constructor would not need to exist.

There may be a few tiny exceptions, but this is not one of them (e.g. if getStats fails here, errors get swallowed).

[1] http://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it

::: dom/media/tests/mochitest/test_peerConnection_stats.html:347
(Diff revisions 14 - 16)
> +        stats.forEach(v => {
> +          if (v.type.endsWith("bound-rtp") && !(v.remoteId)) {
> +            return false;
> +          }
> +        });

There's no way to return from a forEach loop (forEach ignores the result value of the function, and continues the loop). You could use a for-of loop instead.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:357
(Diff revisions 14 - 16)
> +    while (true) {
> +      if (detectRtcp()) {
> +        return resolve();
> +      }
> +      if (totalTime >= 10000) {
> +        return reject("Waiting for RTCP timed out after at least "
> +          + totalTime + "ms");
> +      }
> +      totalTime += waitPeriod;
> +      wait(waitPeriod);
> +    }

This loop busy-waits. `wait(waitPeriod)` is merely a function [1] returning a promise and doesn't block or yield the CPU. It doesn't even wait since you're ignoring the returned promise.

We could make `waitForRtcp` an `async` function and use:

    await wait(waitPeriod);

Or we could recurse using `.then` and use `timeout` function [1] around it to bail.

[1] https://dxr.mozilla.org/mozilla-central/rev/e91de6fb2b3dce9c932428265b0fdb630ea470d7/dom/media/tests/mochitest/head.js#468-471
[2] https://jakearchibald.com/2014/es7-async-functions/
[3] https://dxr.mozilla.org/mozilla-central/rev/e91de6fb2b3dce9c932428265b0fdb630ea470d7/dom/media/tests/mochitest/head.js#485-487
Attachment #8834625 - Flags: review?(jib) → review-
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review118324

> Avoid the promise constructor anti-pattern [1].
> 
> In short, never use the promise constructor except to wrap legacy callback APIs. In other words, in an ideal world where promise-adoption was 100%, the constructor would not need to exist.
> 
> There may be a few tiny exceptions, but this is not one of them (e.g. if getStats fails here, errors get swallowed).
> 
> [1] http://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it

Thank you, this is very good to know.

> There's no way to return from a forEach loop (forEach ignores the result value of the function, and continues the loop). You could use a for-of loop instead.

D'oh, of course.
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review118670

Lgtm with async style nits.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:342
(Diff revisions 16 - 17)
>      let detectRtcp = () => {
> -      pc.getStats().then(stats => {
> +      return pc.getStats().then(stats => {

Async/await replaces promise-chaining, so we might as well go all-async for sub functions inside this async function, for consistency. E.g.:

    let detectRtcp = async () => {
      let stats = await pc.getStats();

::: dom/media/tests/mochitest/test_peerConnection_stats.html:342
(Diff revisions 16 - 17)
>      let detectRtcp = () => {
> -      pc.getStats().then(stats => {
> +      return pc.getStats().then(stats => {

s/=> { return/=>/ ?

::: dom/media/tests/mochitest/test_peerConnection_stats.html:345
(Diff revisions 16 - 17)
> -var waitForRtcp = pc => {
> +var waitForRtcp = async pc => {
> -  return new Promise((resolve,reject) => {
>      let detectRtcp = () => {
> -      pc.getStats().then(stats => {
> -        stats.forEach(v => {
> +      return pc.getStats().then(stats => {
> +        for (let [k, v] of stats) {
>            if (v.type.endsWith("bound-rtp") && !(v.remoteId)) {

Nit: extraneous ()

::: dom/media/tests/mochitest/test_peerConnection_stats.html:346
(Diff revisions 16 - 17)
> -            return false;
> +            return Promise.reject(v.id + " is missing remoteId: "
> +              + JSON.stringify(v));

In mozilla code we try to always reject/throw with an error object. E.g. at least a new Error(str).

Note we could also use `throw` here. Inside a function passed to `.then` and inside `async` functions it has the same effect.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:350
(Diff revisions 16 - 17)
>            if (v.type.endsWith("bound-rtp") && !(v.remoteId)) {
> -            return false;
> +            return Promise.reject(v.id + " is missing remoteId: "
> +              + JSON.stringify(v));
>            }
> +        }
> +        return Promise.resolve(stats);

`Promise.resolve()` is redundant on return values in a function passed to `.then`, as the promise code calling your function does this implicitly. The same is true inside `async` functions.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:354
(Diff revisions 16 - 17)
> -        });
> +      });
> -      });
> -      return true;
>      };
>      let totalTime = 0;
>      let waitPeriod = 500;

const waitPeriod = 500;

::: dom/media/tests/mochitest/test_peerConnection_stats.html:355
(Diff revisions 16 - 17)
> -      });
> -      return true;
>      };
>      let totalTime = 0;
>      let waitPeriod = 500;
>      while (true) {

Maybe:

    for (let totalTime = 0; totalTime < 10000; totalTime += waitPeriod)

?

::: dom/media/tests/mochitest/test_peerConnection_stats.html:356
(Diff revisions 16 - 17)
> -      if (detectRtcp()) {
> -        return resolve();
> +      let found = await detectRtcp().catch((e) => {
> +        info(e);
> +        return undefined;
> +      });

This works, but we should pick a paradigm. In async functions we want to try to use old-style:

    try {
    } catch (e) {
    }

for error handling, and pretend the chaining functions don't exist.

But absence of rtcp doesn't seem like it should be exceptional in a function called detectRtcp() in the first place. Maybe it could return undefined instead?

::: dom/media/tests/mochitest/test_peerConnection_stats.html:364
(Diff revisions 16 - 17)
> -        return reject("Waiting for RTCP timed out after at least "
> +        return Promise.reject("Waiting for RTCP timed out after at least "
>            + totalTime + "ms");

throw new Error("...") inside an async function.
Attachment #8834625 - Flags: review?(jib) → review+
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review118784

Lgtm!

::: dom/media/tests/mochitest/test_peerConnection_stats.html:354
(Diff revisions 17 - 18)
> -        }
> +    }
> -        return Promise.resolve(stats);
> +    return stats;
> -      });
> +  });
> -    };
> -    let totalTime = 0;
> -    let waitPeriod = 500;
> +
> +  const waitPeriod = 500;
> +  for (let totalTime = 10000; totalTime >= 0; totalTime -= waitPeriod) {

I think you mean:

    totalTime > 0
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review118794
Comment on attachment 8834625 [details]
Bug 1337525 - add mochitests for inbound-rtp and outbound-rtp stats;

https://reviewboard.mozilla.org/r/110460/#review118796
Pushed by na-g@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/a9869c358553
add mochitests for inbound-rtp and outbound-rtp stats; r=jib
https://hg.mozilla.org/mozilla-central/rev/a9869c358553
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: