1405591 - Add automated test for the Youtube website

Status: RESOLVED WONTFIX

(Testing :: Firefox UI Tests, enhancement, P5)

Description

[Ioana Crisan]

Link to the TestRail suite: https://testrail.stage.mozaws.net/index.php?/cases/view/3914

Steps:

1. Launch Firefox
2. Navigate to https://www.youtube.com/
3. Sign in
4. Search for a video
5. Scroll down
6. Scroll up
7. Sign out

Note: This test is part of the manual regression suite, hence automation coverage is desirable.

Comment 1

[Ioana Crisan]

Attachment: marionette_youtube_coverage

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

You are aware that you submitted a test with the SV credentials for Youtube? Please change those ASAP.

Comment 3

[Filip Andrei]

Actually this is not a Softvision account, we created this account only for this test.Is that a problem that the password string value contains "Softvision"?because in that case we can change it.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Accounts are always bound to the person who created it. If this account gets abused you are responsible for it. I doubt you want that. So as usual never post any credentials to a public place. And if that happens just change the password, and preferable to a safe one.

Comment 5

[Ioana Crisan]

Attachment: marionette_youtube_coverage_1

Comment 6

[Ioana Crisan]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Accounts are always bound to the person who created it. If this account gets
> abused you are responsible for it. I doubt you want that. So as usual never
> post any credentials to a public place. And if that happens just change the
> password, and preferable to a safe one.

Hi Henrik,

I just changed the password. Do you have rights to delete the obsolete patches? If yes, could you please delete them if you consider this appropriated? Thank you

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We want to discuss this patch in public. So once again, do NOT add credentials to patches! Please change it again, and upload a patch, which has the username and password as *** or whatever.

Comment 8

[Ioana Crisan]

Attachment: marionette_youtube_coverage_2

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

FYI if the patch is ready please request feedback from the people you want to hear their opinion.

Comment 10

[Ioana Crisan]

Could you please point me out the people responsible for doing this?

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Ioana: You can request feedback for attachments in Bugzilla. To do this, click the 'Details' link for the attachment, or visit https://bugzilla.mozilla.org/attachment.cgi?id=8915936&action=edit. You should see an area on the bottom right with dropdown boxes. Click the one with the label "feedback" and select the question mark (?) to show a text box. In the text box type the names of the people you'd like feedback from. As your original email was to Henrik and Maja, you would enter the email addresses associated with their Bugzilla accounts. Fortunately, you can type ":" followed by a few letters of their IRC nicks and Bugzilla will suggest values. For example, Henrik would be suggested by typing ":whim" and Maja by ":maja".

Comment 12

[Ioana Crisan]

(In reply to Dave Hunt (:davehunt) from comment #11)
> Ioana: You can request feedback for attachments in Bugzilla. To do this,
> click the 'Details' link for the attachment, or visit
> https://bugzilla.mozilla.org/attachment.cgi?id=8915936&action=edit. You
> should see an area on the bottom right with dropdown boxes. Click the one
> with the label "feedback" and select the question mark (?) to show a text
> box. In the text box type the names of the people you'd like feedback from.
> As your original email was to Henrik and Maja, you would enter the email
> addresses associated with their Bugzilla accounts. Fortunately, you can type
> ":" followed by a few letters of their IRC nicks and Bugzilla will suggest
> values. For example, Henrik would be suggested by typing ":whim" and Maja by
> ":maja".

Thank you!

Comment 13

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8915936 [details] [diff] [review]
marionette_youtube_coverage_2

Review of attachment 8915936 [details] [diff] [review]:
-----------------------------------------------------------------

If you're just triggering these tests manually, this seems find to me.

::: testing/firefox-ui/tests/functional/security/test_youtube.py
@@ +18,5 @@
> +        self.marionette.navigate(self.url)
> +
> +        # Click the "SIGN IN" button
> +        signInButton = self.marionette.find_element(By.LINK_TEXT, "Sign in")
> +        Wait(self.marionette).until(expected.element_displayed(signInButton))

When you use the `until` function, it's a good idea to provide a message that will be displayed in case of timeout: e.g. Wait(...).until(..., message="Sign-in page did not load.")

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8915936 [details] [diff] [review]
marionette_youtube_coverage_2

Review of attachment 8915936 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I also had a look at this patch and here some comments and questions...

Generally if you want to do any kind of testing for websites I would propose you to use Selenium and geckodriver, and not Marionette. Marionette has a custom protocol and is mainly used for testing Firefox itself.

If you will have multiple tests for a specific domain it might make sense to setup a page object model, which would you require to change ids and such at a single place and not in every test file. But for the beginning this should be fine.

Please note that with the test as written you only can make sure that elements are displayed and work appropriately. You have no chance to check any piece of layout issues.

Given that I do not have any insight in the testrail test I cannot say if all that is enough.

::: testing/firefox-ui/tests/functional/security/test_youtube.py
@@ +13,5 @@
> +
> +        self.url = 'https://www.youtube.com/'
> +
> +    def test_youtube(self):
> +        self.marionette.set_context('content')

The content scope is used by default. There is no need to set it here.

@@ +31,5 @@
> +        nextButton = self.marionette.find_element(By.ID,  "identifierNext")
> +        Wait(self.marionette).until(expected.element_displayed(nextButton))
> +        nextButton.click()
> +
> +        Wait(self.marionette).until(expected.element_not_present(lambda m: m.find_elements(By.ID, "identifierNext")))

This should not be necessary when you wait for the password field to be displayed.

@@ +36,5 @@
> +
> +        # Fill the password
> +        password = self.marionette.find_element(By.ID, "password")
> +        Wait(self.marionette).until(expected.element_displayed(password))
> +        password.send_keys("**********")

If password is getting created with a delay line 38 would still fail. So you really have to add its retrieval to `element_displayed`.

@@ +40,5 @@
> +        password.send_keys("**********")
> +
> +        # Click the "NEXT" button
> +        nextButton = self.marionette.find_element(By.ID,  "passwordNext")
> +        Wait(self.marionette).until(expected.element_displayed(nextButton))

Same as above.

@@ +55,5 @@
> +        searchBarMagnifyingGlass = self.marionette.find_element(By.ID, "search-icon-legacy")
> +        Wait(self.marionette).until(expected.element_displayed(searchBarMagnifyingGlass))
> +        searchBarMagnifyingGlass.click()
> +
> +        Wait(self.marionette).until(expected.element_present(lambda m: m.find_elements(By.CLASS_NAME, "yt-simple-endpoint style-scope ytd-video-renderer")))

Maybe you don't need all the class names.

@@ +60,5 @@
> +
> +        # Identify the 1st video in the page and scroll down until the 8th element is visible
> +        firstVideoInPage = (self.marionette.find_elements(By.CLASS_NAME, "yt-simple-endpoint style-scope ytd-video-renderer"))[0]
> +        Wait(self.marionette).until(expected.element_displayed(firstVideoInPage))
> +        firstVideoInPage.send_keys(u'\ue00f')

Out of interest. Is there a need that such a special unicode character has to be used?

@@ +63,5 @@
> +        Wait(self.marionette).until(expected.element_displayed(firstVideoInPage))
> +        firstVideoInPage.send_keys(u'\ue00f')
> +
> +        # Identify the 8th video in the page and scroll up until the 1st video is visible
> +        eithVideoInPage = (self.marionette.find_elements(By.CLASS_NAME, "yt-simple-endpoint style-scope ytd-video-renderer"))[7]

nit: eight.

@@ +73,5 @@
> +        avatarButton.click()
> +
> +        # Click the "Sign out" button
> +        signOutButton = self.marionette.find_element(By.ID, "endpoint")
> +        signOutButton.click()

If you have to make sure to really sign-out of the website, this code needs to be added to `tearDown`. Otherwise it would not be called, if a test from above fails.

Comment 15

[Ioana Crisan]

(In reply to Henrik Skupin (:whimboo) from comment #14)
> Comment on attachment 8915936 [details] [diff] [review]
> marionette_youtube_coverage_2
> 
> Review of attachment 8915936 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, I also had a look at this patch and here some comments and questions...
> 
> Generally if you want to do any kind of testing for websites I would propose
> you to use Selenium and geckodriver, and not Marionette. Marionette has a
> custom protocol and is mainly used for testing Firefox itself.
> 
> If you will have multiple tests for a specific domain it might make sense to
> setup a page object model, which would you require to change ids and such at
> a single place and not in every test file. But for the beginning this should
> be fine.

These are the only scenarios that we intend to automate for "Youtube" and I don't think it worths to setup a page object. Do you agree?
 
> Please note that with the test as written you only can make sure that
> elements are displayed and work appropriately. You have no chance to check
> any piece of layout issues.
> 
> Given that I do not have any insight in the testrail test I cannot say if
> all that is enough.
> 
> ::: testing/firefox-ui/tests/functional/security/test_youtube.py
> @@ +13,5 @@
> > +
> > +        self.url = 'https://www.youtube.com/'
> > +
> > +    def test_youtube(self):
> > +        self.marionette.set_context('content')
> 
> The content scope is used by default. There is no need to set it here.

If I run the test without setting the context I get the following error on line 31: "UnsupportedOperationException: Only supported in content context". 

> @@ +31,5 @@
> > +        nextButton = self.marionette.find_element(By.ID,  "identifierNext")
> > +        Wait(self.marionette).until(expected.element_displayed(nextButton))
> > +        nextButton.click()
> > +
> > +        Wait(self.marionette).until(expected.element_not_present(lambda m: m.find_elements(By.ID, "identifierNext")))
> 
> This should not be necessary when you wait for the password field to be
> displayed.

Removed the wait.

> @@ +36,5 @@
> > +
> > +        # Fill the password
> > +        password = self.marionette.find_element(By.ID, "password")
> > +        Wait(self.marionette).until(expected.element_displayed(password))
> > +        password.send_keys("**********")
> 
> If password is getting created with a delay line 38 would still fail. So you
> really have to add its retrieval to `element_displayed`.

Tried to check for the presence(expected.element_present) of the "password" field or if the "password" field is displayed(expected.element_displayed) but the test fails. Chose to set a timeout(I saw timeouts were also used in other tests from the same path) and seems to work.

> @@ +40,5 @@
> > +        password.send_keys("**********")
> > +
> > +        # Click the "NEXT" button
> > +        nextButton = self.marionette.find_element(By.ID,  "passwordNext")
> > +        Wait(self.marionette).until(expected.element_displayed(nextButton))
> 
> Same as above.

Added a sleep here too.

> @@ +55,5 @@
> > +        searchBarMagnifyingGlass = self.marionette.find_element(By.ID, "search-icon-legacy")
> > +        Wait(self.marionette).until(expected.element_displayed(searchBarMagnifyingGlass))
> > +        searchBarMagnifyingGlass.click()
> > +
> > +        Wait(self.marionette).until(expected.element_present(lambda m: m.find_elements(By.CLASS_NAME, "yt-simple-endpoint style-scope ytd-video-renderer")))
> 
> Maybe you don't need all the class names.

I shortened the class name used.

> @@ +60,5 @@
> > +
> > +        # Identify the 1st video in the page and scroll down until the 8th element is visible
> > +        firstVideoInPage = (self.marionette.find_elements(By.CLASS_NAME, "yt-simple-endpoint style-scope ytd-video-renderer"))[0]
> > +        Wait(self.marionette).until(expected.element_displayed(firstVideoInPage))
> > +        firstVideoInPage.send_keys(u'\ue00f')
> 
> Out of interest. Is there a need that such a special unicode character has
> to be used?

Indeed, removed the unicode characters.

> @@ +63,5 @@
> > +        Wait(self.marionette).until(expected.element_displayed(firstVideoInPage))
> > +        firstVideoInPage.send_keys(u'\ue00f')
> > +
> > +        # Identify the 8th video in the page and scroll up until the 1st video is visible
> > +        eithVideoInPage = (self.marionette.find_elements(By.CLASS_NAME, "yt-simple-endpoint style-scope ytd-video-renderer"))[7]
> 
> nit: eight.

Corrected the typo.

> @@ +73,5 @@
> > +        avatarButton.click()
> > +
> > +        # Click the "Sign out" button
> > +        signOutButton = self.marionette.find_element(By.ID, "endpoint")
> > +        signOutButton.click()
> 
> If you have to make sure to really sign-out of the website, this code needs
> to be added to `tearDown`. Otherwise it would not be called, if a test from
> above fails.

Added the sign out code to the tearDown.

Comment 16

[Ioana Crisan]

Attachment: marionette_youtube_coverage_3

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Ioana Crisan from comment #15)
Before I start one information for you. When using splinter review by just uploading the patch as attachment, any commenting on code should be done inside this tool. You reach it by clicking the `[review]` link of the attachment. This will make sure that all replies will also be visible in that tool. Just commenting in Bugzilla puts your replies only there.

> > If you will have multiple tests for a specific domain it might make sense to
> > setup a page object model, which would you require to change ids and such at
> > a single place and not in every test file. But for the beginning this should
> > be fine.
> 
> These are the only scenarios that we intend to automate for "Youtube" and I
> don't think it worths to setup a page object. Do you agree?

That's fine then.

> If I run the test without setting the context I get the following error on
> line 31: "UnsupportedOperationException: Only supported in content context". 

Oh wait. If you run those tests via the firefox ui harness then this will be true. But instead just do it via Marionette client itself. That is totally enough given that your tests will not interact with the Firefox UI at all.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8916919 [details] [diff] [review]
marionette_youtube_coverage_3

Review of attachment 8916919 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/firefox-ui/tests/functional/security/test_youtube.py
@@ +21,5 @@
> +        avatarButton.click()
> +
> +        # Click the "Sign out" button
> +        signOutButton = self.marionette.find_element(By.ID, "endpoint")
> +        signOutButton.click()

Just a note. If the test fails before the signin, this might fail. Take this into account when running tests and such a failure occurs.

@@ +31,5 @@
> +        self.marionette.navigate(self.url)
> +
> +        # Click the "SIGN IN" button
> +        signInButton = self.marionette.find_element(By.LINK_TEXT, "Sign in")
> +        Wait(self.marionette).until(expected.element_displayed(signInButton), message="Sign-in page did not load.")

Oh, usually there should be no need to check displayed. If the element would be hidden, a click on it will not succeed. This applies to all of the interactions below too. But if at one point it would be needed, the messages you are using are not correct. You check for displayed and not for existence.

@@ +41,5 @@
> +        username.send_keys("**********")
> +
> +        # Click the "NEXT" button
> +        nextButton = self.marionette.find_element(By.ID,  "identifierNext")
> +        Wait(self.marionette).until(expected.element_displayed(nextButton), message="NEXT button not found.")

You don't check for existence but visibility here.

@@ +44,5 @@
> +        nextButton = self.marionette.find_element(By.ID,  "identifierNext")
> +        Wait(self.marionette).until(expected.element_displayed(nextButton), message="NEXT button not found.")
> +        nextButton.click()
> +
> +        time.sleep(1)

Advise: Never use sleep in your test. Always wait for a condition, which in this case will be password displayed.

@@ +56,5 @@
> +        nextButton = self.marionette.find_element(By.ID,  "passwordNext")
> +        Wait(self.marionette).until(expected.element_displayed(nextButton), message="NEXT button not found.")
> +        nextButton.click()
> +
> +        time.sleep(1)

The click above triggers a page load, right? Then click() will take care of the wait already.

@@ +61,5 @@
> +
> +        # Type "atb" in the search bar
> +        searchBar = self.marionette.find_element(By.ID, "search-input")
> +        Wait(self.marionette).until(expected.element_displayed(searchBar), message="Search bar not found.")
> +        searchBar.send_keys("atb")

Similar as above, "send_keys" will fail if the input field is not visible.

@@ +80,5 @@
> +
> +        # Identify the 8th video in the page and scroll up until the 1st video is visible
> +        eightVideoInPage = (self.marionette.find_elements(By.CLASS_NAME, "yt-simple-endpoint"))[7]
> +        Wait(self.marionette).until(expected.element_displayed(eightVideoInPage), message="Eight video in the page not found.")
> +        eightVideoInPage.send_keys(Keys.PAGE_UP)

would you mind to explain what exactly you are testing here with the scrolling?

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I have two main concerns about these tests. The first is that they will only be testing a very limited set of flows through the websites, and even then will probably only fail if elements are not present or displayed. If we are meaning to use this in place of the web compatibility tests currently in TestRail, then we will not pick up on rendering issues or anything behaving in an unusual way that falls outside the scope of the test. By having someone manually interact with these websites it's much more likely that such things would be noticed.

My other concern is maintainability, particularly as we're writing locators for websites that we have no control over. It's quite usual for websites to change the user interface over time, or to change the DOM so element locators that once worked start to fail. It's also possible that the websites will perform experiments like AB testing, where a percentage of users would see an alternative view of the site. We would have no insight into such things, and would have tests that fail intermittently as a result. I believe that this suite would quickly become a full time job to maintain, and I wonder if the value is worth the cost.

In addition to these concerns, I do not expect that we'd be able to ever run these tests in continuous integration due to the dependency on external resources. This would cause the tests to be flaky not only due to the unknown release schedules and changes for these websites, but also due to the potential availability issues of these resources.

Finally, I would agree with Henrik that Selenium would be a more suitable tool for writing these tests.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

No more progress in the last 2 years, so I assume it's not really needed anymore. Also Firefox ui tests will go away soon via bug 1573410.