Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PeerConnection] Improve getStats WPT test coverage. #26504

Merged
merged 1 commit into from Nov 16, 2020

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Nov 12, 2020

This CL adds two tests that were discovered missing while reviewing the
test coverage, both of which we PASS:

  • getStats(tracks) throws if multiple senders have the same track.
    (Actually it turns out such a test already existed, the old one is
    removed because it was asserting unnecessarily many things.)
  • RTCStats.timestamp should increase with time elapsed.

This CL also makes exsting tests more behavior driven. For example, a
test that advertises itself to test that "report containing
peer-connection stats and outbound-track-stats" should not fail because
we have not implemented RTCCodecStats.sdpFmtpLine - this is a different
behavior. As such, tests that are designed to test the existence of a
stats object are updated only to verify that.
What the old tests were verifying - that the ENTIRE REPORT is valid
and complete - are moved and tested in a separate test that asserts
everything. These tests we still fail because of sdpFmtpLine.

Lastly, the outbound-rtp tests are updated to perform an O/A exchange.
If the senders have not been negotiated to send, there doesn't exist
an encoder or RTP packetizer and as such we shouldn't expect to see
outbound-rtp objects yet.

Bug: chromium:1148286
Change-Id: I50d476f1af41cfae80c77486481e3de06a1d8f5d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2534911
Reviewed-by: Philipp Hancke <philipp.hancke@googlemail.com>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827758}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2534911 branch 3 times, most recently from 5d5b433 to 557952e Compare November 16, 2020 12:01
This CL adds two tests that were discovered missing while reviewing the
test coverage, both of which we PASS:
- getStats(tracks) throws if multiple senders have the same track.
  (Actually it turns out such a test already existed, the old one is
  removed because it was asserting unnecessarily many things.)
- RTCStats.timestamp should increase with time elapsed.

This CL also makes exsting tests more behavior driven. For example, a
test that advertises itself to test that "report containing
peer-connection stats and outbound-track-stats" should not fail because
we have not implemented RTCCodecStats.sdpFmtpLine - this is a different
behavior. As such, tests that are designed to test the existence of a
stats object are updated only to verify that.
  What the old tests were verifying - that the ENTIRE REPORT is valid
and complete - are moved and tested in a separate test that asserts
everything. These tests we still fail because of sdpFmtpLine.

Lastly, the outbound-rtp tests are updated to perform an O/A exchange.
If the senders have not been negotiated to send, there doesn't exist
an encoder or RTP packetizer and as such we shouldn't expect to see
outbound-rtp objects yet.

Bug: chromium:1148286
Change-Id: I50d476f1af41cfae80c77486481e3de06a1d8f5d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2534911
Reviewed-by: Philipp Hancke <philipp.hancke@googlemail.com>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827758}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants