feat(SS): Implement test #121

hristoterezov · 2018-02-12T23:23:24Z

No description provided.

paweldomas · 2018-02-21T21:27:46Z

src/test/java/org/jitsi/meet/test/web/WebParticipantFactory.java

+                }
+                catch (IOException e)
+                {
+                    e.printStackTrace();


what ? shouldn't that fail the test ?

It will eventually fail the test probably anyway. How do you want to change it?

to wrap in RuntimeException and re-throw at least

paweldomas · 2018-02-21T21:29:33Z

src/test/java/org/jitsi/meet/test/web/WebParticipantFactory.java

+        throws IOException
+    {
+        URL website = new URL(
+            EXT_DOWNLOAD_URL_PREFIX + id + EXT_DOWNLOAD_URL_SUFIX);


why not use string format here ?

How? My Java is a little rusty... What will be the benefit?

https://dzone.com/articles/java-string-format-examples

It is weird that there are two constants for something which is actually one thing, where prefix is never used without the suffix.

It's possible that the % signs in the string would have to be escaped.

paweldomas · 2018-02-21T21:30:18Z

src/test/java/org/jitsi/meet/test/web/WebParticipantFactory.java

+        File extension = File.createTempFile("jidesha", ".crx");
+        extension.deleteOnExit();
+
+        FileOutputStream fos = new FileOutputStream(extension);


use https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

Sorry, my java is a little bit rusty as I said. So could you please explain with more details? The part that puzzles me is I want this method to trow the exception.

yes, but the purpose of using that is to close the resource, not to catch the IOException

OK. I'll read the docs more carefully!

paweldomas · 2018-02-21T21:34:32Z

src/test/java/org/jitsi/meet/test/util/MeetUtils.java

+    public static Object getConfigValue(WebDriver participant, String key)
+    {
+        return ((JavascriptExecutor) participant)
+                .executeScript("return config." + key);


I don't like this approach for getting config parameters, because it requires alive and connected to an existing conference WebParticipant. I don't see any of such assumptions being mentioned anywhere around here.

OK. Could you be more specific which approach you would like for this situation? If you don't have better approach in mind, the current approach will be still the best....

Probably at least moving this method to WebParticipant, because having it there means that it belongs to web only.

paweldomas · 2018-02-21T21:36:08Z

Why don't we go with PageObjects given the test is being rewritten ?

hristoterezov · 2018-02-21T22:02:48Z

Why don't we go with PageObjects given the test is being rewritten ?

I have no idea what are you talking about! What is PageObjects?

paweldomas · 2018-02-21T22:12:54Z

PageObjects are used here for example: https://github.com/jitsi/jitsi-meet-torture/blob/master/src/test/java/org/jitsi/meet/test/ChatPanelTest.java

bgrozev · 2018-02-22T16:57:34Z

src/test/java/org/jitsi/meet/test/VideoLayoutTest.java

     */
-    void driverVideoLayoutTest(Participant participant)
+    void driverVideoLayoutTest(Participant participant, boolean isScreenSharing)


I think this method is now superfluous (and the doc needs an update).

The docs are updated! I decided to leave it because in future we may want to add more stuff in it.

The docs are updated!

I don't think this actually made it to the PR:

jitsi-meet-torture/src/test/java/org/jitsi/meet/test/VideoLayoutTest.java

Line 61 in 925dec7

* The webdriver to test.

(this commit is the current head of master)

damencho · 2018-02-23T21:01:25Z

I've added pr testing for torture now. And this one failed:
unknown error: cannot process extension #1
from unknown error: cannot extract magic number
(Driver info: chromedriver=2.35.528139 (47ead77cb35ad2a9a83248b292151462a66cd881),platform=Linux 4.4.0-1050-aws x86_64) (WARNING: The server did not provide any stacktrace information)

We should handle the case if desktop sharing is not configured.

hristoterezov · 2018-02-28T01:48:34Z

@paweldomas I don't see why we need to implement page object for tests that are running only on web. Especially for the desktop sharing test, we don't have any idea if this feature will ever be implemented for mobile and how it will look there. Even if we refactor it now we'll have to definitely refactor it later again when the functionality is implemented for mobile. I think that for desktop sharing test adding page objects will be waste of time (3 times refactoring/rewriting). At the end if there is a desktop sharing for mobile, it can be so different that we may even need totally different test.

I'm sorry if I misunderstood the idea behind the page objects! Please correct me if I'm wrong!

paweldomas · 2018-03-01T02:02:02Z

The ChatPanel test is currently only for web too, so I don't find this argument relevant. I will try to be more specific and comment on each place where that would be suitable and we can continue the discussion from there.

paweldomas · 2018-03-01T02:04:42Z

src/test/java/org/jitsi/meet/test/DesktopSharingTest.java

+        ensureTwoParticipants(
+            null, null, null,
+            new WebParticipantOptions().setChromeExtensionId(
+                (String) MeetUtils.getConfigValue(


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

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.

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

paweldomas · 2018-03-01T02:07:13Z

src/test/java/org/jitsi/meet/test/DesktopSharingTest.java


-        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('"


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

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

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

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.

paweldomas · 2018-03-01T02:09:22Z

src/test/java/org/jitsi/meet/test/DesktopSharingTest.java

+        MeetUIUtils.clickOnToolbarButton(
+            getParticipant1().getDriver(), "toggleFilmstripButton");
+
+        TestUtils.waitMillis(5000);


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.

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.

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

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

paweldomas · 2018-03-01T02:09:57Z

src/test/java/org/jitsi/meet/test/DesktopSharingTest.java

+        MeetUIUtils.clickOnToolbarButton(
+            getParticipant1().getDriver(), "toggleFilmstripButton");
+
+        TestUtils.waitMillis(5000);


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

paweldomas · 2018-03-01T02:17:41Z

src/test/java/org/jitsi/meet/test/DesktopSharingTest.java

+     */
+    private void startDesktopSharing()
+    {
+        TestUtils.waitForElementByXPath(


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.

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.

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

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

paweldomas · 2018-03-01T02:17:55Z

src/test/java/org/jitsi/meet/test/DesktopSharingTest.java

+     */
+    private void stopDesktopSharing()
+    {
+        TestUtils.waitForElementByXPath(


Same as above.

paweldomas · 2018-03-01T02:19:23Z

src/test/java/org/jitsi/meet/test/DesktopSharingTest.java

+    private void checkExpandingDesktopSharingLargeVideo(boolean isScreenSharing)
+    {
+        // check layout
+        new VideoLayoutTest().driverVideoLayoutTest(


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

hristoterezov · 2018-03-01T16:38:20Z

@paweldomas Thanks very much for the additional explanations. Maybe I was not very clear. My problem here is that I don't think we should use the PageObjects for this test. But let's discuss this offline because it seems like a longer discussion.

paweldomas · 2018-03-01T17:08:58Z

@hristoterezov Why would we have two different ways of dealing with things ? Everything is "longer discussion" always. I keep hearing that all the time recently. We can discuss that on the retro or call a meeting.

paweldomas · 2018-03-01T17:10:39Z

@damencho we can do document.getElemntById on web inside of the Button or PageObject abstraction. But on mobile it will be a different way for getting WebElement (find by accessibility ID or something else).

paweldomas · 2018-03-01T17:26:58Z

Btw. long time ago I was forbidden to rewrite whole torture into the PageObject concept and there was an agreement to do that step by step, so here we are. I am really getting frustrated about this attitude of getting shortcuts all the time and then someone else when adding new thing will have to deal with all that. You have just a couple of places to address. I'm not asking you to write full Toolbar or whatever -just the part you need (1 button ?).

paweldomas · 2018-03-02T22:40:51Z

src/test/java/org/jitsi/meet/test/DesktopSharingTest.java

-        TestUtils.waitMillis(5000);
+        TestUtils.waitForDisplayedElementByXPath(
+            getParticipant1().getDriver(),
+            "//span[@id='localVideoWrapper']",


this duplicates the constant and could be a TestUtil method, but I give up since you want to merge it now

bgrozev · 2018-03-03T00:39:34Z

src/test/java/org/jitsi/meet/test/DesktopSharingTest.java

+    private void testDesktopSharingInPresence(final String  expectedResult)
+    {
+        String participant1Jid
+            = MeetUtils.getResourceJid(getParticipant2().getDriver());


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

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

I'm merging anyway and will change it

bgrozev · 2018-03-03T00:42:43Z

src/test/java/org/jitsi/meet/test/DesktopSharingTest.java

+    /**
+     * Stops desktop sharing.
+     */
+    private void stopDesktopSharing()


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

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?

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

hristoterezov requested a review from damencho February 12, 2018 23:25

hristoterezov assigned damencho Feb 12, 2018

feat(SS): Implement test

a491519

damencho force-pushed the ss_test branch from 78d62cf to a491519 Compare February 21, 2018 18:18

damencho approved these changes Feb 21, 2018

View reviewed changes

paweldomas reviewed Feb 21, 2018

View reviewed changes

bgrozev reviewed Feb 22, 2018

View reviewed changes

ref(SS_test): Address review comments

1e740be

paweldomas reviewed Mar 1, 2018

View reviewed changes

feat(Toolbar): Create toolbar page object

3950fbc

damencho force-pushed the ss_test branch from b19f4f4 to a6d74d9 Compare March 2, 2018 22:20

paweldomas reviewed Mar 2, 2018

View reviewed changes

Replaces waits with waiting for local video hiding.

18e3d54

damencho force-pushed the ss_test branch from a6d74d9 to 18e3d54 Compare March 2, 2018 22:56

damencho merged commit 925dec7 into master Mar 3, 2018

bgrozev reviewed Mar 3, 2018

View reviewed changes

hristoterezov deleted the ss_test branch March 3, 2018 00:47

feat(SS): Implement test #121

feat(SS): Implement test #121

Conversation

hristoterezov commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweldomas Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

paweldomas Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweldomas commented Feb 21, 2018

hristoterezov commented Feb 21, 2018

paweldomas commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damencho commented Feb 23, 2018

hristoterezov commented Feb 28, 2018

paweldomas commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hristoterezov commented Mar 1, 2018

paweldomas commented Mar 1, 2018

paweldomas commented Mar 1, 2018 • edited Loading

paweldomas commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweldomas Feb 21, 2018 •

edited

Loading

paweldomas Feb 21, 2018 •

edited

Loading

paweldomas commented Mar 1, 2018 •

edited

Loading