-
Notifications
You must be signed in to change notification settings - Fork 6k
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
test: Connection failure tests #16956
test: Connection failure tests #16956
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very well-documented PR!
await screenshare.startSharing(screenshare.modPage); | ||
|
||
const tcpModeratorSessions = await getCurrentTCPSessions(); | ||
//console.log('tcpModeratorSessions', tcpModeratorSessions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgotten debugging log
// Other comparisons, like == or the array includes method, don't do a deep comparison | ||
// and will always return false since the two arrays contain different objects. | ||
const tcpUserSessions = tcpSessions.filter(x => !tcpModeratorSessions.some(e => deepEqual(e,x))); | ||
//console.log('tcpUserSessions', tcpUserSessions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgotten debugging log
// await notificationsUtil.checkNotificationText(screenshare, "Code 1101. Try sharing the screen again."); | ||
|
||
// but that code only checks the first toast, and there's a bunch of toasts that show up | ||
// on this test. So instead I use xpath, like this: (should we do it this way in checkNotificationText?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Doesn't seem to be a practical issue for checkNotificationText
, but adjusting it the same way could improve the stability of checkNotificationText
, I think.
const tcpSessions = await getCurrentTCPSessions(); | ||
// Other comparisons, like == or the array includes method, don't do a deep comparison | ||
// and will always return false since the two arrays contain different objects. | ||
const tcpUserSessions = tcpSessions.filter(x => !tcpModeratorSessions.some(e => deepEqual(e,x))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the deep-equal's advantage that we are using here? (comparing to Node.js's assert.deepEqual()
) Is it the speed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know we had assert.deepEqual
. I'll take a look at using it instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading some more about this, it looks like assert.deepEqual
will throw an exception rather than just return true or false. As you noted, and as the doc page for deep-equal
notes, deep-equal
is faster than wrapping assert.deepEqual
in a try/catch block.
I think we should add deep-equal
as a new npm dependency (as the PR currently does) and use it just because it's simpler. I use it in a list filter, so it needs to return true or false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
Kudos, SonarCloud Quality Gate passed! |
An initial pair of tests that break TCP sessions during a screen share and check to make sure both the sharer and the viewer get the desired results.
bigbluebutton-tests/playwright/connectionFailure
sudo
access is required and will be prompted for if neededdeep-equal
)screenshare/screenshare.js
class calledMultiUserScreenShare
checkNotificationText