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.

Sign up for GitHub

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

Jump to bottom

feat(SS): Implement test #121

Merged
merged 4 commits into from
Mar 3, 2018
Merged

feat(SS): Implement test #121

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a491519
feat(SS): Implement test
hristoterezov Feb 12, 2018
1e740be
ref(SS_test): Address review comments
hristoterezov Feb 28, 2018
3950fbc
feat(Toolbar): Create toolbar page object
hristoterezov Mar 2, 2018
18e3d54
Replaces waits with waiting for local video hiding.
damencho Mar 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 0 additions & 110 deletions src/test/java/org/jitsi/meet/test/DesktopSharingImitationTest.java
View file
Open in desktop

This file was deleted.

225 changes: 108 additions & 117 deletions src/test/java/org/jitsi/meet/test/DesktopSharingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,12 @@
*/
package org.jitsi.meet.test;

import org.jitsi.meet.test.base.*;
import org.jitsi.meet.test.util.*;
import org.jitsi.meet.test.web.*;

import org.openqa.selenium.*;
import org.openqa.selenium.TimeoutException;
import org.openqa.selenium.support.ui.*;
import org.testng.annotations.*;

import java.io.*;
import java.util.concurrent.*;

import static org.testng.Assert.*;

/**
Expand All @@ -37,128 +31,76 @@ public class DesktopSharingTest
extends WebTestBase
{
/**
* A property to specified the external script that will be used to
* start the new participant.
* The id of the desktop sharing button.
*/
public final static String HOOK_SCRIPT = "desktop.sharing.hook.script";
private static String DS_BUTTON_ID = "toolbar_button_desktopsharing";

/**
* Exception on starting the hook, null if script has started successfully.
* The XPATH of the desktop sharing button.
*/
private Exception hookException = null;
private static String DS_BUTTON_XPATH = "//a[@id='" + DS_BUTTON_ID + "']";

@Override
public boolean skipTestByDefault()
public void setupClass()
{
return true;
super.setupClass();

ensureOneParticipant();

ensureTwoParticipants(
null, null, null,
new WebParticipantOptions().setChromeExtensionId(
(String) MeetUtils.getConfigValue(
Copy link
Member

Choose a reason for hiding this comment

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

Can we move getConfigValue method to WebParticipant ? Then you can call getParticipant1().getConfigValue(). Actually it could event be a method "getDesktopSharingExtensionId()" in the WebParticipantDirectly.

Copy link
Member

Choose a reason for hiding this comment

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

The reason I ask for that is that we're trying to go away from accessing the driver directly and using it with static utility functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

See the previous comments. I've agreed with this and getConfigValue is already a moved to WebParticipant.

getParticipant1().getDriver(),
"desktopSharingChromeExtId")
));
}

/**
* Ensure we have only one participant available in the room - the owner.
* Starts the new participants using the external script and check whether
* the stream we are receiving from him is screen.
* Returns at the end the state we found tests - with 2 participants.
* Check desktop sharing start.
*/
@Test
public void testDesktopSharingInPresence()
public void testDesktopSharingStart()
{
final String hookScript = System.getProperty(HOOK_SCRIPT);

if (hookScript == null)
return;

print("Start testDesktopSharingInPresence.");

ensureOneParticipant();

// Counter we wait for the process execution in the thread to finish
final CountDownLatch waitEndSignal = new CountDownLatch(1);

// counter we wait for indication that the process has started
// or that it ended up with an exception - hookException
final CountDownLatch waitStartSignal = new CountDownLatch(1);

// this will fire a hook script, which needs to launch a browser that
// will join our room
new Thread(() -> {
try
{
JitsiMeetUrl url = getJitsiMeetUrl();

// FIXME the config part may need to by synced up with
// WebParticipant#DEFAULT_CONFIG
url.setHashConfigPart(
"config.requireDisplayName=false"
+ "&config.firefox_fake_device=true"
+ "&config.autoEnableDesktopSharing=true");

String[] cmd = { hookScript, url.toString()};

print("Start the script with param:"+ url);
ProcessBuilder pb = new ProcessBuilder(cmd);
startDesktopSharing();
checkExpandingDesktopSharingLargeVideo(true);
testDesktopSharingInPresence("desktop");

pb.redirectOutput(ProcessBuilder.Redirect.INHERIT);
pb.redirectError(ProcessBuilder.Redirect.INHERIT);
Process p = pb.start();

waitStartSignal.countDown();

p.waitFor();
print("Script ended execution.");
waitEndSignal.countDown();
}
catch (IOException | InterruptedException e)
{
hookException = e;
waitStartSignal.countDown();
}
}).start();

final Participant owner = getParticipant1();

// now lets wait starting or error on startup
try
{
waitStartSignal.await(2, TimeUnit.SECONDS);
if (hookException != null)
{
hookException.printStackTrace();
fail("Error executing hook script:"
+ hookException.getMessage());
}
}
catch (InterruptedException e)
{
e.printStackTrace();
}
}

// let's wait some time the user to joins
TestUtils.waitForBoolean(
owner.getDriver(),
"return (APP.conference.membersCount == 2);",
25);
Participant ownerParticipant = getParticipant1();
ownerParticipant.waitForIceConnected();
ownerParticipant.waitForSendReceiveData();
ownerParticipant.waitForRemoteStreams(1);
/**
* Check desktop sharing stop.
*/
@Test
public void testDesktopSharingStop()
{
stopDesktopSharing();
checkExpandingDesktopSharingLargeVideo(false);
testDesktopSharingInPresence("camera");
}

/**
* Checks the status of desktop sharing received from the presence and
* compares it to the passed expected result.
* @param expectedResult camera/desktop
*/
private void testDesktopSharingInPresence(final String expectedResult)
{
// now lets check whether his stream is screen
String remoteParticipantID = owner.getDriver()
.findElement(By.xpath("//span[starts-with(@id, 'participant_') " +
" and contains(@class,'videocontainer')]")).getAttribute("id");
remoteParticipantID
= remoteParticipantID.replaceAll("participant_", "");
String participant1Jid
= MeetUtils.getResourceJid(getParticipant2().getDriver());
Copy link
Member

Choose a reason for hiding this comment

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

The variable name suggests this is the id of participant 1, but the code gets the id of participant 2. What is the intention?

Copy link
Member Author

Choose a reason for hiding this comment

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

No intention. It's a mistake! I should change the variable name!

Copy link
Member

Choose a reason for hiding this comment

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

I'm merging anyway and will change it


final String expectedResult = "desktop";
// holds the last retrieved value for the remote type
final Object[] remoteVideoType = new Object[1];
try
{
final String scriptToExecute = "return APP.UI.getRemoteVideoType('"
Copy link
Member

Choose a reason for hiding this comment

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

can we make this script execution a "getRemoteVideoType" method in WebParticipant ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there other place where we are using this? I don't think so. That's why I think this belongs to the DesktopSharingTest!

Copy link
Member

Choose a reason for hiding this comment

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

No, it does not ! It belongs to the WebParticipant !

Copy link
Member

Choose a reason for hiding this comment

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

Test class describes what is being tested and how. Who's video type is ? Participant's. In this particular case we're checking what video type given web participant has received for given remote participant through presence. Other tests can be also interested in checking that.

+ remoteParticipantID + "');";
(new WebDriverWait(owner.getDriver(), 5))
+ participant1Jid + "');";
(new WebDriverWait(
getParticipant2().getDriver(), 5))
.until((ExpectedCondition<Boolean>) d -> {
Object res = owner.executeScript(scriptToExecute);
Object res =
getParticipant1().executeScript(scriptToExecute);
remoteVideoType[0] = res;

return res != null && res.equals(expectedResult);
Expand All @@ -167,20 +109,69 @@ public void testDesktopSharingInPresence()
catch (TimeoutException e)
{
assertEquals(
expectedResult, remoteVideoType[0],
"Wrong video type, maybe desktop sharing didn't work");
expectedResult, remoteVideoType[0],
"Wrong video type, maybe desktop sharing didn't work");
}
}

// allow the participant to leave
try
{
waitEndSignal.await(10, TimeUnit.SECONDS);
print("End DesktopSharingTest.");
}
catch (InterruptedException e)
{
e.printStackTrace();
}
ensureTwoParticipants();
/**
* Starts desktop sharing.
*/
private void startDesktopSharing()
{
TestUtils.waitForElementByXPath(
Copy link
Member

Choose a reason for hiding this comment

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

So this 3 statements in the PageObject world would be:
Toolbat toolbar = getParticipant2().getToolbar();
Button dsButton = toolbar.getDesktopsharingButton();
dsButton.click();
dsButton.assertToggled(true /* toggled /, 2 / timeout seconds */);
I don't know what would be the exact type of the Button. Maybe it's just WebElement. Or maybe we need a class the wraps it.

Copy link
Member

Choose a reason for hiding this comment

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

I think the approach with getButton and then click is not good at all in the context of react. I would even say to skip searching for web element and getting them and then executing click. We already had such problems with the toolbars. The better approach is at the end to just execute document.getElementById('...').click();
https://github.com/jitsi/jitsi-meet-torture/blob/master/src/test/java/org/jitsi/meet/test/util/MeetUIUtils.java#L243
The problem is a StaleElementException coming from selenium. You take the element and try to click it, but by that time react has re-rendered the toolbar and that element is stale.

Copy link
Member

Choose a reason for hiding this comment

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

Note that this is a problem with saving the WebElement, and not a problem with the "getButton" abstraction itself.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that's right, thanks for the clarification :)

getParticipant2().getDriver(),
DS_BUTTON_XPATH,
5);
MeetUIUtils.clickOnToolbarButton(getParticipant2().getDriver(),
DS_BUTTON_ID);
TestUtils.waitForElementContainsClassByXPath(
getParticipant2().getDriver(), DS_BUTTON_XPATH,
"toggled", 2);
}

/**
* Stops desktop sharing.
*/
private void stopDesktopSharing()
Copy link
Member

Choose a reason for hiding this comment

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

startDesktopSharing and stopDesktopSharing are exactly the same? Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Kind of. After the refactoring I end up implementing a method that handles both cases. Maybe I should remove the start/stopDesktopSharing and use the new method directly?

Copy link
Member

Choose a reason for hiding this comment

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

I'm merging anyway and replaced the two methods with "toggleDesktopSharing"

{
TestUtils.waitForElementByXPath(
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

getParticipant2().getDriver(),
DS_BUTTON_XPATH,
5);
MeetUIUtils.clickOnToolbarButton(getParticipant2().getDriver(),
DS_BUTTON_ID);
TestUtils.waitForElementNotContainsClassByXPath(
getParticipant2().getDriver(), DS_BUTTON_XPATH,
"toggled", 2);
}

/**
* Checks the video layout on the other side, after we imitate
* desktop sharing.
* @param isScreenSharing <tt>true</tt> if SS is started and <tt>false</tt>
* otherwise.
*/
private void checkExpandingDesktopSharingLargeVideo(boolean isScreenSharing)
{
// check layout
new VideoLayoutTest().driverVideoLayoutTest(
Copy link
Member

Choose a reason for hiding this comment

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

I hate that we need to create test instances somewhere else, but it would be probably too much to refactor this in this PR...

getParticipant1(), isScreenSharing);

// hide thumbs
MeetUIUtils.clickOnToolbarButton(
getParticipant1().getDriver(), "toggleFilmstripButton");

TestUtils.waitMillis(5000);
Copy link
Member

Choose a reason for hiding this comment

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

What are we waiting here for exactly ? Waits like this are waste of time and never check for the condition that is expected after the wait period is over.

Copy link
Member

Choose a reason for hiding this comment

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

We will leave this for now like this. Hiding the filmstrip because of animations can take some time. We cannot check for a css change, cause currently, we change the css and after the change, it starts moving the filmstrip away from the screen for two seconds (which with the framebuffer can take more). We already spend a lot of time looking at it to figure it out. Maybe checking for isDisplayed of the local video is one solution will give it a quick try and if it doesn't work will leave the waits.

Copy link
Member

Choose a reason for hiding this comment

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

So the purpose could be explained in a comment at least.

Copy link
Member

Choose a reason for hiding this comment

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

Seems to wait for local video not displayed works. Will see how it goes. Will merge it after the tests pass.


// check layout
new VideoLayoutTest().driverVideoLayoutTest(
getParticipant1(), isScreenSharing);

// show thumbs
MeetUIUtils.clickOnToolbarButton(
getParticipant1().getDriver(), "toggleFilmstripButton");

TestUtils.waitMillis(5000);
Copy link
Member

Choose a reason for hiding this comment

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

Again: what's the purpose of the wait here ?

}
}
Loading