Closed Bug 1434901 Opened 7 years ago Closed 7 years ago

Remove dependency to element_{top,right,bottom,left}.html in test_visibility.py

Categories

(Remote Protocol :: Marionette, enhancement, P3)

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

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: whimboo, Assigned: csdebugging, 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

(2 files, 2 obsolete files)

test_visibility.py
6.27 KB, text/plain
Details
Bug 1434901 - Remove dependency to element_{top,right,bottom,left}.html in test_visibility.py;
59 bytes, text/x-review-board-request
whimboo
: review+
Details
No longer depends on: 1423661
Blocks: 1423638
Hi, can I work on this bug?
Flags: needinfo?(hskupin)
Sure, let me know if you have questions. Once you have uploaded the patch I will assign the bug to you. Thanks!
Flags: needinfo?(hskupin)
I'm brand new to Mozilla and open source projects. I was hoping, since this is a mentored bug, I could get some help knowing what is what and where things are in the code. As it stands right now, I wouldn't even know where in the code I should look to even start fixing this bug.
(In reply to Henrik Skupin (:whimboo) from comment #2) > Sure, let me know if you have questions. Once you have uploaded the patch I > will assign the bug to you. Thanks! I'm just sending this because I'm not sure if bugzilla gives you a notification that I responded if I don't use "reply."
Please check the user story section above for all the details in how to get started.
Hello Henrik, I was made the commit, and removed the dependencies. If something are wrong, let me know please.
I fixed the bug. How do I submit the patch?
Attached file test_visibility.py —
Flags: needinfo?(hskupin)
(In reply to Benjamin De Brasi from comment #8) > I fixed the bug. How do I submit the patch? Hi Benjamin, as it looks like Mike has uploaded a patch in the meantime already. I missed that he was also working on this bug, but hasn't commented here about that. Sorry. Would you mind to check another bug as marked as dependency for bug 1423638? There are a couple more which are easy to do. I can then explain over there in how to upload your patch. Thanks!
Flags: needinfo?(hskupin)
Attachment #8950399 - Attachment is obsolete: true
Comment on attachment 8950019 [details] Bug 1434901 - Remove html dependency from test cases https://reviewboard.mozilla.org/r/219306/#review225594 The test looks broken because it doesn't contain the following changes: 1) Inclusion of the necessary HTML content into the Python test module 2) The removal of all the no longer needed HTML testcase files Please make sure to check with `hg histedit` which commits are present and squash them all into a single commit, and make sure to upload a complete patch. Thanks. ::: commit-message-8325e:1 (Diff revision 1) > +Bug 1434901 - Remove html dependency from test cases r?whimboo Please specify which HTML testcase file and also test module it is.
Attachment #8950019 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #11) > Comment on attachment 8950019 [details] > Bug 1434901 - Remove html dependency from test cases > > https://reviewboard.mozilla.org/r/219306/#review225594 > > The test looks broken because it doesn't contain the following changes: > > 1) Inclusion of the necessary HTML content into the Python test module > 2) The removal of all the no longer needed HTML testcase files > > Please make sure to check with `hg histedit` which commits are present and > squash them all into a single commit, and make sure to upload a complete > patch. > > Thanks. > > ::: commit-message-8325e:1 > (Diff revision 1) > > +Bug 1434901 - Remove html dependency from test cases r?whimboo > > Please specify which HTML testcase file and also test module it is. Okay, will do it in a few days, sick now.
(In reply to Henrik Skupin (:whimboo) from comment #10) > (In reply to Benjamin De Brasi from comment #8) > > I fixed the bug. How do I submit the patch? > > Hi Benjamin, as it looks like Mike has uploaded a patch in the meantime > already. I missed that he was also working on this bug, but hasn't commented > here about that. Sorry. Would you mind to check another bug as marked as > dependency for bug 1423638? There are a couple more which are easy to do. I > can then explain over there in how to upload your patch. Thanks! This is extremely unfair. I commented on the bug and asked for help on a mentored bug, a system specifically designed to help new comers. I spent hours building Mozilla, reading up on documentation, learning how to traverse a large code base, configuring IRC, making an accounts, etc. all for some random person to abruptly come in and make my work irrelevant. Mike did not comment or indicate in anyway that he's interested in this bug. More importantly, he did not utilize the mentoring aspect at all. He already knew how to do all the things I was just learning how to do as evidenced by the fact that he had already fixed the bug and submitted a patch without asking you a single question. This is totally against the spirit of mentored bugs where it should be people who need mentoring and actively use it should be the priority. More generally, the fact that this can happens alienates new comers in general. What's to stop Mike or other random people from doing the exact same thing for every bug I, or someone just as inexperienced, tries to fix? Nothing, which means that I could try 100 bugs and accomplish nothing and get no mentoring because some person who knows enough to not need a mentor exploits the system and beats every new comer who needs some help getting started to the punch. Having the mentor system be a first come first serve system is antithetical to the idea of nurturing new open source developers. And that harms the Mozilla open source community and new comes alike. I think the fair thing to do would be remove his patch and allow me to work on this bug. The bug you linked me to ALREADY has Mike saying he wants to work on it which is literally showing how the systematic worst case scenario I described above is currently happening. All things equal, Mike will beat me in fixing that bug too or other people will until I figure out how to submit patches on my own making mentored bugs redundant. If you will not or can not reject his patch I'd like to have the contact information of whoever makes the assignee rules and request an official change to the system.
Flags: needinfo?(hskupin)
Hi Benjamin. I can understand your frustration and have to admit that the situation was not handled well. As I already mentioned some days ago Mike talked to me on IRC and showed interest for this bug. But he totally missed to make a comment here. Similar to you he is a new contributor, and actually everyone of us can make mistakes, or forget about something. We should all not be that harsh and fight against each other, but instead work in a friendly way. That is when contribution actually makes most value, also in building your own experience and relationships. Whilst a conversation is ongoing internally in how we could improve the assignment status, I agree that a comment is necessary on the bug to express the interest. Given that this didn't happen here, you are welcome to continue on your work for getting this task done. We will find a different bug for Mike then. Btw the before mentioned bug 1423638 is a tracking bug for various other bugs as linked as dependencies. So no-one can actually work on it. I've made that actually clear. Also this was a bad coincidence because all of your both activity happened on the weekend, where most of us mentors aren't around. So I had no chance to assist earlier, and yes also I forgot about Mike. Does the above help you to understand the situation, and to continue on this bug?
Assignee: nobody → csdebugging
Status: NEW → ASSIGNED
Flags: needinfo?(hskupin)
Attachment #8950399 - Attachment is obsolete: false
Attachment #8950399 - Attachment is patch: false
Attachment #8950019 - Attachment is obsolete: true
If yes, you should have a look at https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html#feature-development which describes in how to upload patches. It should be also linked from within the new contributors guide.
(In reply to Benjamin De Brasi from comment #13) > I spent > hours building Mozilla, reading up on documentation, learning how to > traverse a large code base, configuring IRC, making an accounts, etc. all > for some random person to abruptly come in and make my work irrelevant. While I agree with Henrik's decision to accept your work over Mike's in this case, and I agree with your concerns about welcoming newcomers, I'd like to point out that this incident (regardless of the outcome) does NOT make anyone's learning/configuration irrelevant. You won't ever have to do any of that stuff over again and it's totally worthwhile learning either way: you have more open source experience as a result. The value of open source experience is the whole process (including setup and how you approach conversations with fellow Mozillians) not just the end result of a patch. Next time a miscommunication happens, please keep in mind that collaborating over text-only is a challenge for everyone involved, especially when we span many time zones, languages, cultures, etc. It's easy to jump to conclusions about people's intentions, but we can't let ourselves do that, otherwise we'd be arguing all the time. On this team and at Mozilla as a whole, we assume everyone's best intentions all around unless someone is being overtly aggressive or disrespectful. When frustration comes up, we stay calm and think twice about how to express ourselves. Thanks for taking the time to contribute to our project. We're happy to help you learn. Looking forward to your finished patch. :)
Hello, sorry guys, it's maybe my fault that I didn't commented the bug, and it's a general reason, why this misunderstanding happened.
I'm sorry if my tone was hostile. I was quite discouraged upon seeing the updates yesterday. I also did not mean to ascribe specific motivations to anyone. I tried to keep the comment as objective as possible. Thank you Maja and Henry for your quick responses and open minded resolution. I will also take the advice you've given to heart.
(In reply to Henrik Skupin (:whimboo) from comment #15) > If yes, you should have a look at > https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/ > firefoxworkflow.html#feature-development which describes in how to upload > patches. It should be also linked from within the new contributors guide. I think my error right now is installing Mercurial. When I was building Firefox I could not install the latest version of Mercurial and every time I tried to do it with pip it would fail. I used apt instead and when I try to use Mercurial, the terminal keeps informing that I should get the most recent version, but installation keeps failing. Would you be able to help me update it or point me where I can get help configuring it?
Flags: needinfo?(hskupin)
I've also tried to install from source and it has given me errors.
We might be able to help with more info. Maybe link to a pastebin (https://pastebin.mozilla.org/) with info about your environment (os version), the commands you're running and the errors you get with pip, etc.
Benjamin, so you executed `./mach bootstrap`? It usually installs all the requirements you need, unless you have Windows which uses MozillaBuild.
Flags: needinfo?(hskupin)
I actually was able to install and configure Mercurial (I think) on my own. I have version 4.4.2 and I'm running Ubuntu 16.04 in case that information becomes relevant later. I am trying to push test_visibility.py for review using command 'hg push review', but that produced the following text on my terminal: pushing to https://reviewboard-hg.mozilla.org/autoreview searching for appropriate review repository redirecting push to https://reviewboard-hg.mozilla.org/gecko (ignoring public changeset c4c9b0c5796b in review request) abort: no non-public changesets left to review (add or change the -r argument to include draft changesets) It seems 'hg push review' is the first step according to multiple documentation web pages I visited so I am unsure how to proceed. More generally, I am not familiar with Mercurial (I have experience with git) so I don't know the work flow and have no sense of context for what I'm supposed to do, in what order and what may go wrong.
Flags: needinfo?(mjzffr)
Flags: needinfo?(hskupin)
I assume you've already run ./mach mercurial-setup read this: https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html#feature-development (As a small addition to that workflow, I recommend you create a bookmark, which is like a git branch, whenever you start new work: hg pull; hg up central; hg bookmark myfeature; <make changes>; hg commit; hg push review. You can always get back to the tip of your bookmark with `hg up myfeature`.) When do you |hg push review|, you should be at the most-recent commit of your work (think of it as the "tip" of a git branch). If you installed |hg wip| with ./mach mercurial-setup, you can use it to orient yourself. It's hard to troubleshoot this sort of error over Bugzilla. Maybe try to ask whimboo or me for help in #ateam. If we're not online when you are, there are likely helpful people on #introduction or #developers who can help you out in more detail. I think questions about Mercurial are actually pretty common in those channels. Keep us posted, good luck.
Flags: needinfo?(mjzffr)
As Maja said you most likely did not create a bookmark for your work as suggested by the docs, but committed your change directly on tip of central. Best is to export the top commit, and then strip it off. After creating the bookmark you can import your formerly exported patch again. Then a push to review should work. Note that using bookmarks also let you work on multiple things in parallel, and you do not clutter your copy of the public branch/bookmark.
Flags: needinfo?(hskupin)
Attached patch rb220780.patch (obsolete) — — Splinter Review
Comment on attachment 8951482 [details] [diff] [review] rb220780.patch Marking as obsolete in favour of MozReview request.
Attachment #8951482 - Attachment is obsolete: true
Comment on attachment 8951478 [details] Bug 1434901 - Remove dependency to element_{top,right,bottom,left}.html in test_visibility.py; https://reviewboard.mozilla.org/r/220780/#review226706
I see on the review "Some jobs failed on try." Am I supposed to fix that? If so, how can I see which test failed?
Flags: needinfo?(hskupin)
"See it in Treeherder" > Click on any orange "jobs" to see logs > click on various buttons in the bottom panel to see additional details. Doing the above, I get to this: https://treeherder.mozilla.org/logviewer.html#?job_id=162573669&repo=try&lineNumber=258
Flags: needinfo?(hskupin)
Comment on attachment 8951478 [details] Bug 1434901 - Remove dependency to element_{top,right,bottom,left}.html in test_visibility.py; https://reviewboard.mozilla.org/r/220778/#review227016
Hi, I'm very confused on how to fix the bug that is generated from the review site. I thought importing 'print_function' and eliminating print statements I used to debug would do it, but this error still shows up. "TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py:5:1 | Missing from __future__ import print_function (require print_function)" When I run './mach marionette test' on my build the following occurs: 'Ran 917 checks (917 tests) Expected results: 869 Skipped: 46 tests Unexpected results: 2 test: 2 (2 fail) Unexpected Logs --------------- testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestPosition.test_move_to_negative_coordinates FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestPosition.test_move_to_negative_coordinates testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestPosition.test_set_position_with_rect FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestPosition.test_set_position_with_rect' Also, can you tell me the difference between those two test sets?
Flags: needinfo?(hskupin)
FYI, Henrik is unavailable for the next few days. Just importing print_function and leaving everything else as is should be enough. The failure you see on MozReview is a check of python3 compatibility. You can run it locally with |./mach lint -l py2 -l py3 testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py|
Flags: needinfo?(hskupin)
(In reply to Benjamin De Brasi from comment #40) > Hi, I'm very confused on how to fix the bug that is generated from the > review site. I thought importing 'print_function' and eliminating print > statements I used to debug would do it, but this error still shows up. > > "TEST-UNEXPECTED-ERROR | > /builds/worker/checkouts/gecko/testing/marionette/harness/marionette_harness/ > tests/unit/test_visibility.py:5:1 | Missing from __future__ import > print_function (require print_function)" Just a drive-by given that Maja already replied on that. If you see this failure it means you should still have a print statement somewhere in that specific test module. If that is really not the case I would consider that a bug in the linter. But please check if you have really moved all those debug print statements. I believe there is at least one left.
Thank you for informing me of Henrik's lack of availability. I ran |./mach lint -l py2 -l py3 testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py| and the following came up: ✖ 0 problems (0 errors, 0 warnings) I changed nothing about test_visibility.py. I also inspected the file for print statements and I couldn't find any. Is there a reason why it would pass the tests locally and not on the review site?
Flags: needinfo?(mjzffr)
Flags: needinfo?(hskupin)
Comment on attachment 8951478 [details] Bug 1434901 - Remove dependency to element_{top,right,bottom,left}.html in test_visibility.py; https://reviewboard.mozilla.org/r/220780/#review227602 ::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py:5 (Diff revision 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 > +from __future__ import absolute_import, print_function Since you removed the print statement from `elementDirectionTest`, you don't need to import `print_function` anymore.
The review site ran tests on your initial submission, which did contain a print statement, which is why that test failed.
Flags: needinfo?(mjzffr)
Flags: needinfo?(hskupin)
(In reply to Maja Frydrychowicz (:maja_zf) (away 26/02-02/03) from comment #45) > The review site ran tests on your initial submission, which did contain a > print statement, which is why that test failed. So the error was caused by merely importing 'print_function'? It makes sense to not import unnecessary libraries, but I thought the error was telling me I needed to import the 'print_function' rather than get rid of it. I didn't think a test could fail because a library was imported. Also, I submitted a version without the 'print_function' and without any print statements.
(In reply to Benjamin De Brasi from comment #47) > (In reply to Maja Frydrychowicz (:maja_zf) (away 26/02-02/03) from comment > #45) > > The review site ran tests on your initial submission, which did contain a > > print statement, which is why that test failed. > > So the error was caused by merely importing 'print_function'? It makes sense > to not import unnecessary libraries, but I thought the error was telling me > I needed to import the 'print_function' rather than get rid of it. I didn't > think a test could fail because a library was imported. > > Also, I submitted a version without the 'print_function' and without any > print statements. Sorry, I somehow and embarrassingly misread your very clear statement. I was just very sure one of the versions I submitted before this had no print statements and that importing an unnecessary library was causing a test fail. Whenever you have a chance, I'd like to know what the next steps I need to take for this bug are, if any.
Flags: needinfo?(mjzffr)
Comment on attachment 8951478 [details] Bug 1434901 - Remove dependency to element_{top,right,bottom,left}.html in test_visibility.py; https://reviewboard.mozilla.org/r/220780/#review228164 Thank you for the update of the patch. It looks better but would still need a couple of updates. Please see my inline comments. ::: commit-message-994a8:1 (Diff revision 9) > +Bug 1434901 - Remove dependency to element_{top,right,bottom,left}.html in test_visibility.py; r?whimboo I don't see that you have removed any of those HTML testcase files. ::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py:11 (Diff revision 9) > > from marionette_driver.by import By > > from marionette_harness import MarionetteTestCase > > +import urllib This is a global import, which should be listed first but still after future imports. See https://www.python.org/dev/peps/pep-0008/#imports Please run `mach lint testing/marionette` which should show a failure, and some more as stated below. ::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py:13 (Diff revision 9) > > from marionette_harness import MarionetteTestCase > > +import urllib > + > +def inline(doc): Around global functions you will have to add 2 empty lines. The linter will fail otherwise. ::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py:16 (Diff revision 9) > +import urllib > + > +def inline(doc): > + return "data:text/html;charset=utf-8,{}".format(urllib.quote(doc)) > + > +def elementDirectionTest(direction): It's not a test but delivers testing content. I would prefer a name like `element_direction_doc()`. ::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py:18 (Diff revision 9) > +def inline(doc): > + return "data:text/html;charset=utf-8,{}".format(urllib.quote(doc)) > + > +def elementDirectionTest(direction): > + code = inline(""" > +<!DOCTYPE html> You can remove this line. Also for a better reading experience you can better intend those HTML lines with 8 chars. It won't affect the rendering. ::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py:30 (Diff revision 9) > + height: 100px; > + }} > +</style> > +<div class='element'></div> > +""".format(direction)) > + return code You can return immediately without the extra declaration of `code`. ::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py:127 (Diff revision 9) > child = self.marionette.find_element(By.ID, 'child') > self.assertFalse(child.is_displayed()) > > def testShouldClickOnELementPartiallyOffLeft(self): > - test_html = self.marionette.absolute_url("element_left.html") > - self.marionette.navigate(test_html) > + test_html = self.marionette.navigate(elementDirectionTest("left")) > + #self.marionette.navigate(test_html) Since `navigate` doesn't return a URL, you want to remove the variable in all the cases below, and also the commented out line.
Attachment #8951478 - Flags: review?(hskupin) → review-
I think I changed everything you pointed out. Let me know if anything is wrong.
Flags: needinfo?(hskupin)
Thanks Benjamin, I will have a look soon. FYI when you update a patch the review flag is set again. If that is done there is no need to set an extra need-info flag. I will see it anyway.
Flags: needinfo?(hskupin)
Comment on attachment 8951478 [details] Bug 1434901 - Remove dependency to element_{top,right,bottom,left}.html in test_visibility.py; https://reviewboard.mozilla.org/r/220780/#review228588 r=me with the trailing spaces fixed. I will also trigger a try build to see if all is still passing. ::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py:14 (Diff revisions 9 - 11) > > from marionette_harness import MarionetteTestCase > > -import urllib > > -def inline(doc): > +def inline(doc): When you check the code changes in mozreview, please note those red blocks, which indicate that there are whitespace issues. Please fix those two instances here to make the linter happy.
Attachment #8951478 - Flags: review?(hskupin) → review+
Comment on attachment 8951478 [details] Bug 1434901 - Remove dependency to element_{top,right,bottom,left}.html in test_visibility.py; https://reviewboard.mozilla.org/r/220780/#review227602 > Since you removed the print statement from `elementDirectionTest`, you don't need to import `print_function` anymore. Maja, can you please close this issue? It is fixed. Thanks.
Comment on attachment 8951478 [details] Bug 1434901 - Remove dependency to element_{top,right,bottom,left}.html in test_visibility.py; https://reviewboard.mozilla.org/r/220780/#review228608 ::: testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py:126 (Diff revision 11) > self.marionette.navigate(test_html) > child = self.marionette.find_element(By.ID, 'child') > self.assertFalse(child.is_displayed()) > > def testShouldClickOnELementPartiallyOffLeft(self): > - test_html = self.marionette.absolute_url("element_left.html") > + test_html = self.marionette(element_direction_doc("left")) You stripped away the `.navigate()` call, which causes test failures for everything. https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd900d81ecb5df8a0fbd2c579eb20641f03179c1&selectedJob=163912613 Basically you should always run the test and ensure that everything works before updating the patch on mozreview.
Attachment #8951478 - Flags: review+ → review-
I corrected what you pointed out and ran |./mach marionette test| and two tests failed that seem unrelated. Here's the output for the tests: marionette-test ~~~~~~~~~~~~~~~ Ran 917 checks (917 tests) Expected results: 869 Skipped: 46 tests Unexpected results: 2 test: 2 (2 fail) Unexpected Logs --------------- testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestPosition.test_move_to_negative_coordinates FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestPosition.test_move_to_negative_coordinates testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestPosition.test_set_position_with_rect FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestPosition.test_set_position_with_rect
Flags: needinfo?(hskupin)
Yes, that looks fine now. Please upload those changes. Thanks.
Flags: needinfo?(hskupin)
I ran |./mach marionette test| twice and I still get the 2 errors that you said were fine, but on the review board I have 6 errors which also seem much more severe in type. I'm really at a lost as to why running the same files on my computer would get one set of errors and the review site running the same code would have another. I'm also unsure how to proceed from this point.
Flags: needinfo?(hskupin)
Try builds are not run automatically but need to be triggered manually. The failures you have seen came from your former patch, which had the problems I mentioned above. Given that we run the tests on multiple platforms, failures get multiplied if visible everywhere. I triggered a new try build now. Results should be available soon.
Flags: needinfo?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #62) > Try builds are not run automatically but need to be triggered manually. The > failures you have seen came from your former patch, which had the problems I > mentioned above. Given that we run the tests on multiple platforms, failures > get multiplied if visible everywhere. I triggered a new try build now. > Results should be available soon. Thank you for telling me that! I was quite frustrated trying to figure what was going on. I'll remember that for future patches. I see the results now and there's one test that failed, but it doesn't look related to 'test_visibility.py' file. I'm not 100% how to interpret it. Also, in general should I just wait for changes on the review site instead of commenting here?
Flags: needinfo?(hskupin)
Comment on attachment 8951478 [details] Bug 1434901 - Remove dependency to element_{top,right,bottom,left}.html in test_visibility.py; https://reviewboard.mozilla.org/r/220780/#review229034
Attachment #8951478 - Flags: review?(hskupin) → review+
Comment on attachment 8951478 [details] Bug 1434901 - Remove dependency to element_{top,right,bottom,left}.html in test_visibility.py; https://reviewboard.mozilla.org/r/220780/#review227602 > Maja, can you please close this issue? It is fixed. Thanks. Ben, if Maja is not around can you please close this issue yourself? I'm not able to land your patch otherwise. Thanks.
(In reply to Henrik Skupin (:whimboo) from comment #65) > Comment on attachment 8951478 [details] > Bug 1434901 - Remove dependency to element_{top,right,bottom,left}.html in > test_visibility.py; > > https://reviewboard.mozilla.org/r/220780/#review227602 > > > Maja, can you please close this issue? It is fixed. Thanks. > > Ben, if Maja is not around can you please close this issue yourself? I'm not > able to land your patch otherwise. Thanks. I closed the issue.
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1905fd787278 Remove dependency to element_{top,right,bottom,left}.html in test_visibility.py; r=whimboo
Does that push mean I'm done with this issue? Does my patch get added to the code base now or is there more of a review process?
It has landed on an integration branch. There could still be a risk of a backout if something goes wrong. But I don't think this will happen. So yes, the work is all done on this bug. Thanks for working on it. Let me know, as best via IRC, if you are interested in something else, maybe more challenging.
Flags: needinfo?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #69) > It has landed on an integration branch. There could still be a risk of a > backout if something goes wrong. But I don't think this will happen. So yes, > the work is all done on this bug. Thanks for working on it. Let me know, as > best via IRC, if you are interested in something else, maybe more > challenging. Great! Thanks for all the help. I will definitely follow up.
https://hg.mozilla.org/mozilla-central/rev/1905fd787278
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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: