Closed Bug 1423899 Opened 7 years ago Closed 5 years ago

Remove dependency to pageSource* HTML testcase files in test_pagesource.py

Categories

(Remote Protocol :: Marionette, enhancement, P3)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement

Tracking

(firefox71 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox71 --- fixed

People

(Reporter: whimboo, Assigned: rgpt, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=py])

User Story

For steps in how to get started please consult our documentation for new users:
https://firefox-source-docs.mozilla.org/testing/marionette/marionette/NewContributors.html

In the case of questions you can find us on IRC in the #ateam channel.

Attachments

(1 file, 6 obsolete files)

Bug 1423899 - Removing HTML Dependencies in test_pagesource.py
47 bytes, text/x-phabricator-request
Details | Review
Summary: Remove dependency to blob_download.html in test_window_handles_content.py (test_window_handles_after_opening_new_non_browser_window) → Remove dependency to pageSource* HTML testcase files in test_pagesource.py
I am not sure if I understand it correctly. The current test is using an existing HTML file in order to run its tests. You want the test itself to create an html file with minimal tags in order to run it's tests( using the "inline" method) and then delete it? If that is the case can I get assigned to it, if not could you clarify what I understood wrong?

Thank you in advance.
Flags: needinfo?(hskupin)
I adapted the test to use 'inline' and pushed it for review.

https://reviewboard.mozilla.org/r/206766/

I haven't deleted the provided TestPageSources, as I am not sure if they still needed.
I will attach a screenshot of a successful marionette test run as well. :)
Attached image page_source_test.png (obsolete) — 
3 out of 3 test PASS
I'm a student from coventry university studying an opensource module, I would like to be assigned this as my first bug! Could you please point me in the right direction of how to start
I already did this one mate,sorry, and the Reporter isn't responding.
(In reply to Kristiyan from comment #1)
> I am not sure if I understand it correctly. The current test is using an
> existing HTML file in order to run its tests. You want the test itself to
> create an html file with minimal tags in order to run it's tests( using the
> "inline" method) and then delete it? If that is the case can I get assigned
> to it, if not could you clarify what I understood wrong?

Yes, this is all right. If the HTML file is no longer used by any other Python unit test in the same folder it can be deleted. Please check that.
Assignee: nobody → dimitr14
Status: NEW → ASSIGNED
Flags: needinfo?(hskupin)
This GREP of the test directory shows that the test files were only used by /home/firefox-dev/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py. Therefore they are now redundant and can be removed.
Attachment #8936456 - Flags: feedback+
Comment on attachment 8935893 [details]
[mq]: Bug 1423899 - Cleaning up now redundant test files(3).

https://reviewboard.mozilla.org/r/206766/#review213106

This patch does no longer contain the changes to test_pageSource.py. Please make sure that all the changes are part of the same commit. Also please make sure to set a valid commit message as described at https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#
Attachment #8935893 - Flags: review-
Will do, I wasn't aware that all the changes need to be part of one commit.
Thank you.
Where did my commit with test_pageSource.py go?? It has vanished I can't even see it on mozreview. I don't have a solid backup, I didn't know they can just delete things. There is an automatic backup created from 'hg strip tip --force" but I can't find how to use it. Could you help me please.
Flags: needinfo?(hskupin)
I don't know what you did before pushing the update to mozreview. But basically it will not remove former changes. So whether you were working on a different bookmark/branch for the update, or a history rewrite was not successful. Whatever, you can thankfully find the changed content on the left side of the interdiff viewer for version 1 vs 2:

https://reviewboard.mozilla.org/r/206766/diff/1-2/
Flags: needinfo?(hskupin)
[Mass Change 2018-01-15] Moving bugs to backlog
Priority: -- → P3
Krisiyan, do you have an update for your patch? It would be great to get this finally landed. Thanks.
Flags: needinfo?(dimitr14)
Sadly no response from the original author. So we are looking for someone else to continue the work.
Assignee: dimitr14 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(dimitr14)
i am working on it.
I am getting what to do but really don't how to do. @whimboo may you explain how should i proceed to bring change  in test_pagesource.py,
Flags: needinfo?(hskupin)
Kamit, please have a look at my comment 0 for test_typing. You would have to add such a `inline` method, which then accepts any kind of HTML content. Just check that other test file in how to use `inline`, and use the HTML content from the references HTML testcase files, which then can be removed.
Flags: needinfo?(hskupin)
If nobody works on the bug, I'll happy to work on the bug.
Flags: needinfo?(hskupin)
Mike, you already got some experiences with good first bugs. I think it is time to have something more challenging for you and leave those bugs for starters. I will be back on Thursday this week, so we could discuss on IRC. Does it sound ok?
Flags: needinfo?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #21)
> Mike, you already got some experiences with good first bugs. I think it is
> time to have something more challenging for you and leave those bugs for
> starters. I will be back on Thursday this week, so we could discuss on IRC.
> Does it sound ok?

Yes, it's sounds good. Looking forward a message from you.
I am sorry, did not see the messages. My solution is in
https://reviewboard.mozilla.org/r/206766/diff/1-2/ and I thought this would suffice and that I didn't need to push again[as from comment 13]. It is working and tested.
Flags: needinfo?(hskupin)
Kristiyan, the commit https://reviewboard.mozilla.org/r/206766/diff/1-2/ actually reverted all the changes as you previously did for the test module. You want to revert those changes, and push again.
Flags: needinfo?(hskupin)
Assignee: nobody → dimitr14
Status: NEW → ASSIGNED
I will do it, it might take some time as my setup broke and I need to re-setup everything. Ideally will be done in Monday as I am working over the Weekend. Sorry for the inconvenience.
Attachment #8935893 - Attachment is obsolete: true
I assume the updated patch is ready for review? If that is the case please set the reviewer in mozreview in the future. I will have a look now.
Comment on attachment 8954251 [details]
[mq]: Bug-1423899 Remove dependency to pageSource* HTML testcase files in test_pagesource.py (and remove reduntant files).

https://reviewboard.mozilla.org/r/223402/#review229478

Thank you for updating the patch! I have made a couple of comments for improvements. Please have a look at those. Otherwise this version contains everything what we need now. Thanks.

::: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py:2
(Diff revision 1)
> +# -*- coding: utf-8 -*-
> +# Encoding needed for function: testShouldReturnTheSourceOfAPageWhenThereAreUnicodeChars

There is no need to add this comment.

::: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py:12
(Diff revision 1)
>  
> +
>  from marionette_harness import MarionetteTestCase
>  
>  
> +import urllib

This is a global package and would need to be imported first. Also please make sure to pull the latest changes from mozilla-central given that the list of imports has been changed meanwhile, and you will see merge conflicts.

::: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py:28
(Diff revision 1)
>          self.assertTrue("<html" in source)
>          self.assertTrue("PageSource" in source)
>          self.assertEqual(source, from_web_api)
>  
>      def testShouldReturnTheSourceOfAPageWhenThereAreUnicodeChars(self):
> -        test_html = self.marionette.absolute_url("testPageSourceWithUnicodeChars.html")
> +        self.marionette.navigate(inline('<html><head><meta charset="utf-8"/>\

The charset is already defined in the `inline()` method. Try to reduce the HTML content to the only necessary parts, like stripping off all the `<head>` content.

To circumvent the encoding issue you could also replace `»` with the Unicode escape code.

::: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py:38
(Diff revision 1)
>          source = self.marionette.page_source
>          from_web_api = self.marionette.execute_script("return document.documentElement.outerHTML")
>          self.assertEqual(source, from_web_api)
>  
>      def testShouldReturnAXMLDocumentSource(self):
> -        test_xml = self.marionette.absolute_url("testPageSource.xml")
> +        page_insert = '<xml><foo><bar>baz</bar></foo></xml>'

The `inline()` function defines `text/html` which will not work in this case. Maybe you can modify that method to additionally accept a MIME type, and default to `text/html`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py:38
(Diff revision 1)
>          source = self.marionette.page_source
>          from_web_api = self.marionette.execute_script("return document.documentElement.outerHTML")
>          self.assertEqual(source, from_web_api)
>  
>      def testShouldReturnAXMLDocumentSource(self):
> -        test_xml = self.marionette.absolute_url("testPageSource.xml")
> +        page_insert = '<xml><foo><bar>baz</bar></foo></xml>'

Lets name this variable `content`

::: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py:43
(Diff revision 1)
> -        test_xml = self.marionette.absolute_url("testPageSource.xml")
> -        self.marionette.navigate(test_xml)
> +        page_insert = '<xml><foo><bar>baz</bar></foo></xml>'
> +        self.marionette.navigate(inline(page_insert))
>          source = self.marionette.page_source
>          from_web_api = self.marionette.execute_script("return document.documentElement.outerHTML")
> -        import re
> -        self.assertEqual(re.sub("\s", "", source), "<xml><foo><bar>baz</bar></foo></xml>")
> +        # Checking specifically for XML source
> +        self.assertTrue(page_insert in source )

This should be `assertEqual()` too.

::: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py:44
(Diff revision 1)
> -        self.marionette.navigate(test_xml)
> +        self.marionette.navigate(inline(page_insert))
>          source = self.marionette.page_source
>          from_web_api = self.marionette.execute_script("return document.documentElement.outerHTML")
> -        import re
> -        self.assertEqual(re.sub("\s", "", source), "<xml><foo><bar>baz</bar></foo></xml>")
> +        # Checking specifically for XML source
> +        self.assertTrue(page_insert in source )
> +        # Compare fully 

You can remove this comment
Attachment #8954251 - Flags: review-
Hi Kristiyan, do you have an update for us? It would be great when we can finish this patch and get it landed. Thanks.
Flags: needinfo?(dimitr14)
Sadly we didn't receive any response from Kristiyan. Because of that I will put back this bug into the new state, and everyone else who want to continue to work on the patch is welcome to do so.
Assignee: dimitr14 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(dimitr14)
Can I work on this bug?
I am also working on another bug, but I have submitted the patch for that and needs a review. Am I allowed to work on multiple bugs? 

As I understand, I need to change the HTML files' references to inline function calls, and the tests check whether the web API property(?) document.documentElement.outerHTML must return the page's source code. Why are multiple assert statements needed in first and third test?
Flags: needinfo?(hskupin)
Aditya, you already worked on two good first bugs and I would suggest that we find something more challenging for you. Good first bugs are meant to be assigned to first-time contributors, but you already passed this step. Please send me an email and point out in what you are interested in, then we can find a good next bug for you. Thanks!
Flags: needinfo?(hskupin)
Alright!

I have emailed you the relevant details. Thanks!
Hi!!! Can I work on this bug??
(In reply to janamejay.keskar from comment #34)
> Hi!!! Can I work on this bug??

Totally. I would be happy to mentor you in getting this bug fixed. Let me know if you have issues.
(In reply to Henrik Skupin (:whimboo) from comment #35)
> (In reply to janamejay.keskar from comment #34)
> > Hi!!! Can I work on this bug??
> 
> Totally. I would be happy to mentor you in getting this bug fixed. Let me
> know if you have issues.

Yeah, I will definitely let you know if I have issues.
is anyone working on this? Can i take this bug?
(In reply to Ritvik rajvanshi from comment #37)
> is anyone working on this? Can i take this bug?

I am working on this bug
I have made the changes and I have tested test_pagesource.py. What should I do next??
Janamejay, as I already mentioned on IRC please read the documentation as linked to in the user story at the top of this bug.
Attachment #8982281 - Flags: review?(hskupin)
Comment on attachment 8982281 [details]
Bug 1423899 - Remove dependency to pageSource* HTML testcase files in test_pagesource.py

https://reviewboard.mozilla.org/r/248228/#review254594

Thank you for the patch! I have had a look and added a couple of comments. Please let me know if something isn't clear for you.

Also please note that all the HTML files need to be removed. Maybe you missed to run `hg rm %path%` for each of them?

When updating your patch please make sure to use `hg commit --amend` so that the changes will be part of the original commit as uploaded here.

::: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py:11
(Diff revision 1)
>  
> +import urllib
> +
>  from marionette_harness import MarionetteTestCase
>  
> +def inline(doc):

Please run `mach lint testing/marionette`. You will see that the linter should complain about missing lines. Means top-level methods/classes need 2 empty lines as separator.

::: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py:23
(Diff revision 1)
> +            <html>
> +                <body>
> +                    <p> Check the PageSource
> +                </body>
> +            </html>
> +        """))

You can reduce the HTML content as much as possible. It will be fine if you just use `<body><p>foo</p></body>`. It will even fit in a single line.

Once done make sure to also update the asserts below.

::: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py:37
(Diff revision 1)
> -        self.marionette.navigate(test_html)
> +            <!DOCTYPE html>
> +            <html>
> +                <meta http-equiv="pragma" content="no-cache"/>
> +                </body>
> +            </html>
> +        """))

This HTML content is not what is used in the HTML file. First you have to specify the UTF-8 encoding, and second we also need such a character added. Feel free to use the cookie emoji (https://unicode-table.com/de/1F36A/) and adding it here via the HTML-Code. For verifying it in Marionette the test should use the Unicode number. See `test_profile_management.py` in the same folder in how to use those numbers which start with `\u`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py:44
(Diff revision 1)
>          source = self.marionette.page_source
>          from_web_api = self.marionette.execute_script("return document.documentElement.outerHTML")
>          self.assertEqual(source, from_web_api)
>  
>      def testShouldReturnAXMLDocumentSource(self):
> -        test_xml = self.marionette.absolute_url("testPageSource.xml")
> +        content = '<xml><foo><bar>baz</bar></foo></xml>' 

nit: trailing space the linter should warn you about.

::: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py:49
(Diff revision 1)
> -        test_xml = self.marionette.absolute_url("testPageSource.xml")
> -        self.marionette.navigate(test_xml)
> +        content = '<xml><foo><bar>baz</bar></foo></xml>' 
> +        self.marionette.navigate(inline(content))
>          source = self.marionette.page_source
>          from_web_api = self.marionette.execute_script("return document.documentElement.outerHTML")
>          import re
> -        self.assertEqual(re.sub("\s", "", source), "<xml><foo><bar>baz</bar></foo></xml>")
> +        self.assertEqual(re.sub("\s", "", source), "<html><head></head><body><xml><foo><bar>baz</bar></foo></xml></body></html>")

We don't need the regular expression here anymore given that you perfectly reformatted it into a single line.

Also do not copy the expected content into the second argument but use string formatting via `.format()` to insert the text from `content`.
Attachment #8982281 - Flags: review?(hskupin) → review-
Comment on attachment 8982281 [details]
Bug 1423899 - Remove dependency to pageSource* HTML testcase files in test_pagesource.py

https://reviewboard.mozilla.org/r/248228/#review255420

::: package-lock.json:9
(Diff revision 2)
>    "lockfileVersion": 1,
>    "dependencies": {
>      "acorn": {
> -      "version": "5.5.3",
> -      "resolved": "https://registry.npmjs.org/acorn/-/acorn-5.5.3.tgz",
> -      "integrity": "sha512-jd5MkIUlbbmb07nXH0DT3y7rDVtkzDi4XZOUVWAer8ajmF/DTSSbl5oNFyDOl/OXA33Bl79+ypHhl2pN20VeOQ=="
> +      "version": "5.6.1",
> +      "resolved": "https://registry.npmjs.org/acorn/-/acorn-5.6.1.tgz",
> +      "integrity": "sha512-XH4o5BK5jmw9PzSGK7mNf+/xV+mPxQxGZoeC36OVsJZYV77JAG9NnI7T90hoUpI/C1TOfXWTvugRdZ9ZR3iE2Q=="

Those changes should not be part of your patch. As best check the diff before publishing an update of the patch on mozreview.

::: package.json:7
(Diff revision 2)
>    "name": "mozillaeslintsetup",
>    "description": "This package file is for setup of ESLint only for editor integration.",
>    "repository": {},
>    "license": "MPL-2.0",
>    "dependencies": {
> -    "eslint": "4.19.1",
> +    "eslint": "^4.19.1",

Same for this file.
Attachment #8982281 - Flags: review?(hskupin) → review-
Comment on attachment 8982281 [details]
Bug 1423899 - Remove dependency to pageSource* HTML testcase files in test_pagesource.py

https://reviewboard.mozilla.org/r/248228/#review255426

Oh, I also do not see any updates to my review comments which I have added about 4 days ago. Please make sure to check and fix those.
Hi, 
I have never contributed to an Open Source before, but willing to be a part of it. Can anyone please help me, where do I work, and how do I start ? I did read a few documentations but still haven't been able to figure things out. 
Also, I don't seem to find any bugs to work on. I am trying to do something in python but looks like all bugs are assigned. What do I do ?
Thank you,
Janamejay, will you be able to finish this patch? If not please let us know so that JJ could take over. Thanks.
Flags: needinfo?(janamejay.keskar)
(In reply to Henrik Skupin (:whimboo) from comment #47)
> Janamejay, will you be able to finish this patch? If not please let us know
> so that JJ could take over. Thanks.

Hi. I will not be able to finish the patch.
Flags: needinfo?(janamejay.keskar)
Thank you Janamejay. JJ, feel free to pick this bug up. You can also have a look at the following URL, which I just updated because due to a change the location has been changed:

(Commenting on User Story)
> For steps in how to get started please consult our documentation for new
> users:
> https://firefox-source-docs.mozilla.org/testing/marionette/doc/marionette/
> NewContributors.html

Let me know if there is something else which is missing to get you started. Thanks.
User Story: (updated)
Flags: needinfo?(madalmandal97)
Hi Henrik, Thank you very much, I'll start looking through the documentation.
Flags: needinfo?(madalmandal97)
Hi, I am new to this. Can anyone help me in picking up this bug?
I believe Janamejay is looking at this currently.
Sorry, I am working on this.
(In reply to JJ from comment #53)
> Sorry, I am working on this.

It's been 20 days since you mentioned that you work on it. Can you please share the current status of your work? When will a first version of the patch be ready to get uploaded? Without a patch in the next week we might let someone else work on it. Thanks.
Flags: needinfo?(madalmandal97)
Hi, I am sorry I won't be able to finish it.
Flags: needinfo?(madalmandal97)
Devika, do you still have interest to finish up the patch on this bug?
Flags: needinfo?(devikasugathan007)
Yeah, I would like to work on it.
Flags: needinfo?(devikasugathan007)
Attachment #8954251 - Attachment is obsolete: true
That's great! Please have a look at the user story at the top in how to get started. Then you can already take most of the changes from the last attached patch (marked with yellow background). Let me know if you have problems.
User Story: (updated)
If Devika Sugathan decides to not go through with this bug, then i would like to work on it.
Yeah, you can take it as I'm busy with my exams.
Attachment #8936456 - Attachment is obsolete: true
Attachment #8935896 - Attachment is obsolete: true
Adarsh, that is good to hear. I will assign this bug to you once a patch got uploaded.
Ok
I actually didnt have a very thorough look at the bug before asking to work on it. Now that i have looked at it, it looks well above the range of my skill set. So TLDR I will not be able to submit a patch. Sorry
Adarsh, this bug is actually not that hard to do. We will be here to mentor you in case you have still interest to work on it. Please let me know.

To sum up. The patch as already written and attached needs the review comments from comment 42 and comment 44 be addressed.
Can I  take it up as my exams got over. I'm also not sure on how to proceed but I'll try my best.
Ok I will try to work on it as soon as i get finished with figuring out mercurial.
Devika, lets give Adarsh a chance to work on this bug. Thanks. Adarsh, let me know if you have problems with Mercurial, so we can get those fixed. As best join us on irc.mozilla.org in the #ateam channel in case of questions.
(Commenting on User Story)
> For steps in how to get started please consult our documentation for new
> users:
> https://firefox-source-docs.mozilla.org/testing/marionette/marionette/
> NewContributors.html
> 

The link for how to submit patches is broken in this documentation. So could you point me to  different documentation
Hi! I am a new contributor and am interested to work on this bug. Please let me know if I could work on this if it's still open?
Ben asked me a week ago to work on this patch. If he doesn't respond within the next two days please feel to to get started.
Flags: needinfo?(csdebugging)
I will be working on this bug.
Flags: needinfo?(csdebugging) → needinfo?(hskupin)
Benjamin, on what specifically do you request info from me? I don't see any question. So please go ahead and once the patch has been uploaded I will review it. Please note that you need phabricator now. See (https://moz-conduit.readthedocs.io/) how to set it up.
Flags: needinfo?(hskupin)
I just wanted to make sure everything was ok for me to work on this bug.
Attached patch Bug1423899.patch (obsolete) — — Splinter Review 

        Attachment #9009045 -
        Flags: review?(hskupin)

    
    

    
  
I've been working on this bug, Anushi. Had some errors with my Firefox build that I've been working on with Henrik and was only able to fix yesterday. I will be submitting my own patch soon.

    
    

    
  
Hi Benjamin, Apologies I didn't ask before, I thought it has been 11 days so you might not be working anymore.Please do mark my patch as obsolete :)
Flags: needinfo?(csdebugging)

    
    

    
  
Thank you. Good luck finding another bug!
Flags: needinfo?(csdebugging)

        Attachment #9009045 -
        Attachment is obsolete: true

        Attachment #9009045 -
        Flags: review?(hskupin)

    
    

    
  
Hi everyone, Can I give it a try?

    
    

    
  
The time we are waiting for Benjamin to fix the bug has outgrown our usual time frame for mentored bug. 

Anushi, we marked your patch as obsolete before. Would you mind to continue on it if I review it? If you don't have time and interest I would like to hand it over to soniyavyas29. Thanks.
Flags: needinfo?(anushimaheshwari95)

    
    

    
  
:whimboo Yes please review it :)
Flags: needinfo?(anushimaheshwari95)

        Attachment #9009045 -
        Attachment is obsolete: false

        Attachment #9009045 -
        Flags: review?(hskupin)

    
    

    
  
Perfect. I will have a look at it soon.
Assignee: nobody → anushimaheshwari95
Status: NEW → ASSIGNED

    
    

    
  
Comment on attachment 9009045 [details] [diff] [review]
Bug1423899.patch

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

The files which need to be changed and removed are all part of the patch, which is great. But please see the inline comments, which need to be fixed first.

Also please try to submit to phabricator if possible. See https://moz-conduit.readthedocs.io/ in how to set it up. Thanks!

::: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py
@@ +19,5 @@
> +                <body>
> +                  <p> Check the PageSource
> +                </body>
> +                </html>
> +            """))

Please use an indentation of 2 characters here. As such move the whole block to the left. Make sure to also run the linter via `./mach lint testing/marionette`.

Note that this applies to all three tests.

@@ +31,5 @@
>      def testShouldReturnTheSourceOfAPageWhenThereAreUnicodeChars(self):
> +        test_html = self.marionette.navigate(inline("""
> +                <html>
> +                  <head>
> +                    <meta charset="utf-8"/>

No need for this line given that this is already set by inline.

@@ +34,5 @@
> +                  <head>
> +                    <meta charset="utf-8"/>
> +                    <meta http-equiv="pragma" content="no-cache"/>
> +                    <!--
> +                      - the « section[id^="wifi-"] » selector.

Did you verify that this test works? If you run it Python would blame that this file hasn't been marked with a utf-8 encoding, because those two letters aren't ASCII.

To solve this you have to add the encoding, or encode those two characters eg. `\ue007`.

        Attachment #9009045 -
        Flags: review?(hskupin) → review-

    
    

    
  
Anushi, do you have an update for us?
Flags: needinfo?(anushimaheshwari95)

    
    

    
  
I'm going to start working on this bug as per our email exchange, Henrik.

    
    

    
  
I have a fix I'd like to test, but I can't find information on testing my code. Which commands do I need to run?


Flags: needinfo?(hskupin)

    
    

    
  
Benjamin, please check https://firefox-source-docs.mozilla.org/testing/marionette/Testing.html#marionette-functional-tests in how to run the unit tests for Marionette.


Flags: needinfo?(hskupin)
Assignee: anushimaheshwari95 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(anushimaheshwari95)

    
    

    
  
One of the test cases are failing and I don't know how to fix it. It seems the "source" variable which is given it's value by "self.marionette.page_source" in the "testShouldReturnReturnAXMLDocumentSource" method is adding some beginning html tags to the solely expected xml code resulting in an error. This is the output of running "./mach marionette-test testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py":


./mach marionette-test testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py


0:00.00 INFO Using workspace for temporary data: "/home/ben/BigData/src/mozilla-unified"

0:00.00 mozversion INFO application_buildid: 20190323155725

0:00.00 mozversion INFO application_changeset: 59e55930dc0f243357a8730be1a0ca372e6baddb

0:00.00 mozversion INFO application_display_name: Nightly

0:00.00 mozversion INFO application_id: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}

0:00.00 mozversion INFO application_name: Firefox

0:00.00 mozversion INFO application_remotingname: firefox

0:00.00 mozversion INFO application_vendor: Mozilla

0:00.00 mozversion INFO application_version: 68.0a1

0:00.00 mozversion INFO platform_buildid: 20190323155725

0:00.00 mozversion INFO platform_changeset: 59e55930dc0f243357a8730be1a0ca372e6baddb

0:00.00 mozversion INFO platform_version: 68.0a1

0:00.00 INFO Application command: /home/ben/BigData/src/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/bin/firefox -no-remote -marionette -profile /tmp/tmpkuwpjU.mozrunner

0:02.02 INFO Profile path is /tmp/tmpkuwpjU.mozrunner

0:02.02 INFO Starting fixture servers

0:02.14 INFO Fixture server listening on http://127.0.0.1:41567/

0:02.14 INFO Fixture server listening on https://127.0.0.1:33217/

0:02.34 INFO e10s is enabled

0:02.36 mozversion INFO application_buildid: 20190323155725

0:02.36 mozversion INFO application_changeset: 59e55930dc0f243357a8730be1a0ca372e6baddb

0:02.36 mozversion INFO application_display_name: Nightly

0:02.36 mozversion INFO application_id: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}

0:02.36 mozversion INFO application_name: Firefox

0:02.36 mozversion INFO application_remotingname: firefox

0:02.36 mozversion INFO application_vendor: Mozilla

0:02.36 mozversion INFO application_version: 68.0a1

0:02.36 mozversion INFO platform_buildid: 20190323155725

0:02.36 mozversion INFO platform_changeset: 59e55930dc0f243357a8730be1a0ca372e6baddb

0:02.36 mozversion INFO platform_version: 68.0a1

0:02.36 SUITE_START: marionette-test - running 1 tests

0:02.36 TEST_START: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py TestPageSource.testShouldReturnAXMLDocumentSource

0:02.61 TEST_END: FAIL, expected PASS - AssertionError: u'<html><head></head><body><xml><foo><bar>baz</bar></foo></xml></body></html>' != '<xml><foo><bar>baz</bar></foo></xml>'

Traceback (most recent call last):

File "/home/ben/BigData/src/mozilla-unified/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 159, in run

testMethod()

File "/home/ben/BigData/src/mozilla-unified/testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py", line 66, in testShouldReturnAXMLDocumentSource

self.assertEqual(re.sub("\s", "", source), "<xml><foo><bar>baz</bar></foo></xml>")


0:02.62 TEST_START: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py TestPageSource.testShouldReturnTheSourceOfAPage

0:02.66 TEST_END: PASS

0:02.66 TEST_START: testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py TestPageSource.testShouldReturnTheSourceOfAPageWhenThereAreUnicodeChars

0:02.71 TEST_END: PASS

0:02.71 INFO

SUMMARY


0:02.71 INFO passed: 2

0:02.71 INFO failed: 1

0:02.71 INFO todo: 0

0:02.71 INFO

FAILED TESTS


0:02.71 INFO test_pagesource.py test_pagesource.TestPageSource.testShouldReturnAXMLDocumentSource

0:02.71 SUITE_END


marionette-test


Ran 3 checks (3 tests)
Expected results: 2
Unexpected results: 1
  test: 1 (1 fail)

Unexpected Results
------------------
FAIL testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py TestPageSource.testShouldReturnAXMLDocumentSource - AssertionError: u'<html><head></head><body><xml><foo><bar>baz</bar></foo></xml></body></html>' != '<xml><foo><bar>baz</bar></foo></xml>'
Traceback (most recent call last):
  File "/home/ben/BigData/src/mozilla-unified/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 159, in run
    testMethod()
  File "/home/ben/BigData/src/mozilla-unified/testing/marionette/harness/marionette_harness/tests/unit/test_pagesource.py", line 66, in testShouldReturnAXMLDocumentSource
    self.assertEqual(re.sub("\s", "", source), "<xml><foo><bar>baz</bar></foo></xml>")



Flags: needinfo?(hskupin)

    
    

    
  
Sorry, but as long as I cannot see your changes to the code there is nothing I can do.


Flags: needinfo?(hskupin)

    
    

    
  
-- coding: utf-8 --


This Source Code Form is subject to the terms of the Mozilla Public


License, v. 2.0. If a copy of the MPL was not distributed with this


file, You can obtain one at http://mozilla.org/MPL/2.0/.


from future import absolute_import


import urllib


from marionette_harness import MarionetteTestCase


def inline(doc):

return "data:text/html;charset=utf-8,{}".format(urllib.quote(doc))


class TestPageSource(MarionetteTestCase):

def testShouldReturnTheSourceOfAPage(self):

#test_html = self.marionette.absolute_url("testPageSource.html")

self.marionette.navigate(inline("""

<html>

<head>

<title>PageSource Test</title>

</head>

<body>

<p> Check the PageSource

</body>

</html>

"""))

source = self.marionette.page_source

from_web_api = self.marionette.execute_script("return document.documentElement.outerHTML")

self.assertTrue("<html" in source)

self.assertTrue("PageSource" in source)

self.assertEqual(source, from_web_api)


def testShouldReturnTheSourceOfAPageWhenThereAreUnicodeChars(self):
    #test_html = self.marionette.absolute_url("testPageSourceWithUnicodeChars.html")
    self.marionette.navigate(inline("""
				<html>
				  <head>
					  <meta charset="utf-8"/>
					  <meta http-equiv="pragma" content="no-cache"/>
					  <!--
  					    - the « section[id^="wifi-"] » selector.
  					  -->
				  </body>
				</html>
				"""))
    # if we don't throw on the next line we are good!
    source = self.marionette.page_source
    from_web_api = self.marionette.execute_script("return document.documentElement.outerHTML")
    self.assertEqual(source, from_web_api)

def testShouldReturnAXMLDocumentSource(self):
    #test_xml = self.marionette.absolute_url("testPageSource.xml")
    self.marionette.navigate(inline("""
                                    <xml>
				  <foo>
					    <bar>baz</bar>
				  </foo>
				</xml>
				"""))
    source = self.marionette.page_source
    from_web_api = self.marionette.execute_script("return document.documentElement.outerHTML")
    import re
    self.assertEqual(re.sub("\s", "", source), "<xml><foo><bar>baz</bar></foo></xml>")
    self.assertEqual(source, from_web_api)



Flags: needinfo?(hskupin)

    
    

    
  
Your inline() method as defined always produces HTML. If you want to have XML, you would need something like:

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/support/inline.py


Flags: needinfo?(hskupin)

    
    

    
  
I don't understand the example and can't find documentation on the various methods and variable names used in inline for that example nor can I find documentation on .format() itself. Could you explain or link to documentation that can explain what's going on?


Flags: needinfo?(hskupin)

    
    

    
  
Basically what you want is to serve a different doctype for XML otherwise it won't work as you can see. Check the example for exactly this purpose. Also format() is a method which comes from Python itself. So check its own documentation.


Flags: needinfo?(hskupin)

    
    

    
  
If no one else is working on it, we are a student group from LNMIIT learning the Team Software Process, and if you are able to mentor us can we pick up this enhancement? @Henrik



    
    

    
  
Kabeer, at any time! Just read the user story at the top of the bug, and especially the link for new contributors. So get familiar with Marionette, and how to run tests. Feel free to ask if something is unclear, here or on IRC.



    
    

    
  
@Henrik All right we will start getting familiar with the work that has been done and other stuff that you mentioned thanks!



    
    

    
  
We are still actually working on this @Rishi Gupta , I do think we have a 20 day period before someone else is allowed to work on this, can you confirm @Henrik


Flags: needinfo?(hskupin)

    
    

    
  
I'm not aware of a 20 days period. If we don't here from people after a week, or don't see any progress it is fine if someone else gets also started. The bug will usually be assigned to the person who first submits a usable patch. So if you are still interested in contributing please feel free to pick another bug which we have lots of. Thanks.


Assignee: nobody → rishigpt2009
Status: NEW → ASSIGNED
Flags: needinfo?(hskupin)

    
    

    
  
Hi! I'm new to open source contribution and I'd like to give this a try if no one else is working on it currently.



    
    

    
  
No, this bug is taken. Please have a look at others like bug 1443572 or bug 1354458.



    
    

    
  
We are a student group from a college. We had picked up this bug as part of our course project. We had to document the process by which we will fix this bug which is why we were late in submitting a patch. We have already created the whole schema for patching this bug and were actually going to submit it by today or tomorrow. So if possible @Rishi Gupta, @Henrik could you please reconsider assigning this bug to us.

If not then can you please point us towards other good first bugs related to python.

Thank you.


The plan we formulated:

https://imgur.com/a/yV0SKAI


Flags: needinfo?(rishigpt2009)
Flags: needinfo?(hskupin)

        Attachment #8982281 -
        Attachment is obsolete: true
Flags: needinfo?(hskupin)

        Attachment #9009045 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Kabeer Seth from comment #102)




We are a student group from a college. We had picked up this bug as part of our course project. We had to document the process by which we will fix this bug which is why we were late in submitting a patch. We have already created the whole schema for patching this bug and were actually going to submit it by today or tomorrow. So if possible @Rishi Gupta, @Henrik could you please reconsider assigning this bug to us.

If not then can you please point us towards other good first bugs related to python.




We have guidelines in how to handle mentored bugs. And as you can see a lot tries have been made before which all were not successful, or we never got reply from the person who wanted to fix it. So by default it's a first come, first serve policy, and the bug will be assigned to the person who uploads the patch at first. Please see my comment 101 which references two easy bugs to work on. Those are not exactly the same, but easy to implement. I hope that's still ok for you.


Flags: needinfo?(rishigpt2009)

    
    

    
  
@whimboo resolved nits on D45958. Thanks



    
    

    
  
There was no reply by Kabeer in the last 8 days. So I think that we are fine in getting review for Rishi's patch, and get it landed once finished.



    
    

    
  
I didn't reply anything because I thought you have assigned it to Rishi and was trying to look and understand thew new bugs that you stated above, if you are allowing me to work on this I can submit a patch in 24 hours after you allow me to.

@Whimboo


Flags: needinfo?(hskupin)

    
    

    
  
Actually not for this bug anymore. The patch is close to be landed. Please reply on one of the other bugs in a timely manner if you want to work on it, and also supply a patch without a huge delay. Feel happy to ask on such a bug if something is unclear.


Flags: needinfo?(hskupin)

    
    

    
  
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de702b56d454
Removing HTML Dependencies in test_pagesource.py r=whimboo

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/de702b56d454


Status: ASSIGNED → RESOLVED
Closed: 5 years ago

          status-firefox71:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

    
  
Product: Testing → Remote Protocol

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: