Closed
Bug 1348145
Opened 8 years ago
Closed 6 years ago
Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize
Categories
(Remote Protocol :: Marionette, enhancement, P3)
Tracking
(firefox-esr60 fixed, firefox61 fixed, firefox62 fixed)
RESOLVED
FIXED
mozilla62
People
(Reporter: automatedtester, Assigned: vpit3833, Mentored)
References
Details
(Whiteboard: [lang=py][lang=js])
Attachments
(1 file, 7 obsolete files)
Since there needs to be some period where we don't break people's selenium tests, I am leaving this bug to remove those methods at some stage (probably 56 or 57).
Reporter | ||
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•7 years ago
|
||
I like to work on this. How to find out which methods qualify to be removed?
Flags: needinfo?(dburns)
Comment 2•7 years ago
|
||
Please see the following code for Marionette server and client: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver.js#3645 https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver.js#3647 https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#1385-1393 https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#2045-2062 Also you will have to make sure that all Marionette unit tests are getting updated and pass.
Flags: needinfo?(dburns)
Updated•7 years ago
|
Summary: Remove setWIndowPosition and setWindowSize → Remove setWindowPosition and setWindowSize
Updated•7 years ago
|
Mentor: hskupin
status-firefox60:
--- → affected
status-firefox61:
--- → affected
Whiteboard: [lang=py][lang=js]
Assignee | ||
Comment 3•7 years ago
|
||
I am looking for a way to replace set_window_position calls in test_window_rect.py with set_window_rect before removing the definition of set_window_position. What values of height and width do I use in setup method of test_window_rect.py? There is no get_window_rect defined in marionette.py. The closest I can find is in /testing/web-platform/tests/webdriver/tests/get_window_rect.py and doesn't look like what can be used here. So, for the moment, I am thinking between setting up height and width with hand written constants or define get_window_rect in marionette.py.
Flags: needinfo?(hskupin)
Assignee | ||
Comment 4•7 years ago
|
||
I might have answered my own question here. Please comment on this and once everything is ironed out, I will upload a full patch removing setWindowSize as well.
Flags: needinfo?(hskupin)
Attachment #8965538 -
Flags: feedback?(hskupin)
Comment 5•7 years ago
|
||
I will have a look in a moment. But beside that please note that we also have to remove the getters. I updated the summary accordingly.
Assignee: nobody → venkateshpitta
Status: NEW → ASSIGNED
Summary: Remove setWindowPosition and setWindowSize → Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize
Comment 6•7 years ago
|
||
Comment on attachment 8965538 [details] [diff] [review] bug-1348145-attempt-01.patch Review of attachment 8965538 [details] [diff] [review]: ----------------------------------------------------------------- The changes mostly touch the right code, but please see my inline comments in what would need to be improved here. ::: testing/marionette/client/marionette_driver/marionette.py @@ +1383,4 @@ > DeprecationWarning) > return self._send_message("getWindowPosition") > > + def get_window_rect(self): There is no need to add another method here. Support for getting the window rect is already present by the `window_rect` property. @@ +1393,1 @@ > def set_window_position(self, x, y): You want to update this method. I think it's better to keep it to ease the setting of the window position. That way you don't necessarily have to use the `set_window_rect` method directly. For example see our webdriver client: https://dxr.mozilla.org/mozilla-central/rev/2f5ffe4fa2153a798ed8b310a597ea92abd1b868/testing/web-platform/tests/tools/webdriver/webdriver/client.py#260-273 ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py @@ -12,4 @@ > > def setUp(self): > MarionetteTestCase.setUp(self) > - self.original_position = self.marionette.get_window_position() In driver.js you removed `setWindowPosition`, so this is not necessary to change unless you plan to also remove `getWindowPosition` in the same commit. Maybe you do two commits, one for window position and the other for window size. @@ +75,5 @@ > + self.marionette.get_window_rect())) > + self.marionette.set_window_rect(-8, -8, 8, 8) > + position = self.marionette.get_window_rect() > + print("Position after requesting move to negative coordinates and dimensions: {}" > + .format(position)) This test should only test moving the window but not resizing.
Attachment #8965538 -
Flags: feedback?(hskupin) → feedback+
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #6) > ::: testing/marionette/client/marionette_driver/marionette.py > @@ +1383,4 @@ > > DeprecationWarning) > > return self._send_message("getWindowPosition") > > > > + def get_window_rect(self): > > There is no need to add another method here. Support for getting the window > rect is already present by the `window_rect` property. Done. Removed the new method and started using the property. > > @@ +1393,1 @@ > > def set_window_position(self, x, y): > > You want to update this method. I think it's better to keep it to ease the > setting of the window position. That way you don't necessarily have to use > the `set_window_rect` method directly. > > For example see our webdriver client: > > https://dxr.mozilla.org/mozilla-central/rev/ > 2f5ffe4fa2153a798ed8b310a597ea92abd1b868/testing/web-platform/tests/tools/ > webdriver/webdriver/client.py#260-273 > Sorry, I am still not clear. Do I use @command pattern on set_window_position to have it call set_window_rect? > ::: > testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py > @@ -12,4 @@ > > > > def setUp(self): > > MarionetteTestCase.setUp(self) > > - self.original_position = self.marionette.get_window_position() > > In driver.js you removed `setWindowPosition`, so this is not necessary to > change unless you plan to also remove `getWindowPosition` in the same commit. > > Maybe you do two commits, one for window position and the other for window > size. I am attaching the edits to remove set/get window_position in the next comment/attachment. > > @@ +75,5 @@ > > + self.marionette.get_window_rect())) > > + self.marionette.set_window_rect(-8, -8, 8, 8) > > + position = self.marionette.get_window_rect() > > + print("Position after requesting move to negative coordinates and dimensions: {}" > > + .format(position)) > > This test should only test moving the window but not resizing. ok, now passing the original height/width along and ignoring the two. How can this be done better?
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8965538 -
Attachment is obsolete: true
Attachment #8965926 -
Flags: feedback?(hskupin)
Comment 9•7 years ago
|
||
(In reply to Venkatesh from comment #7) > > @@ +1393,1 @@ > > > def set_window_position(self, x, y): > > > > You want to update this method. I think it's better to keep it to ease the > > setting of the window position. That way you don't necessarily have to use > > the `set_window_rect` method directly. > > > > For example see our webdriver client: > > > > https://dxr.mozilla.org/mozilla-central/rev/ > > 2f5ffe4fa2153a798ed8b310a597ea92abd1b868/testing/web-platform/tests/tools/ > > webdriver/webdriver/client.py#260-273 > > Sorry, I am still not clear. Do I use @command pattern on > set_window_position to have it call set_window_rect? No, I proposed to just use a call to `set_window_rect` with only the `x` and `y` coordinates specified. But now I had another look at the Python client code and as it looks like we are free to specify whatever we want. So I wonder if we should keep the old methods and enforce the appropriate parameters, or remove them and loosely handle sizing and positioning via `set_window_rect`. Personally I would prefer the former. Andreas, what do you think? > > > + self.marionette.get_window_rect())) > > > + self.marionette.set_window_rect(-8, -8, 8, 8) > > > + position = self.marionette.get_window_rect() > > > + print("Position after requesting move to negative coordinates and dimensions: {}" > > > + .format(position)) > > > > This test should only test moving the window but not resizing. > > ok, now passing the original height/width along and ignoring the two. How > can this be done better? `set_window_rect` currently accepts None for both width and heigh, so you could leave them out. But lets wait until the above question has been answered.
Flags: needinfo?(ato)
Comment 10•7 years ago
|
||
I have in the past raised concern about removing set_window_position and set_window_size from the client API we offer to end-users. The WebDriver:SetWindowRect command is meant as a performance improvement to reduce the number of driver requests, and unless youβre testing the WebDriver protocol (as the WPT client does) it is more user friendly to have the client API match what the user wants to do: whilst it is possible to atomically reposition and resize the window I donβt know how many users that is actually a useful primitive for. Now I donβt care about this so strongly that I want to discourage the attached patched, but at the same time I donβt think the WPT WebDriver is a good source of inspiration here since it is geared more towards testing the low-level protocol than it is towards providing an idiomatic and user-friendly API. In any case, please feel free to move forward with the current plans.
Flags: needinfo?(ato)
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Andreas Tolfsen βΉ:atoβΊ from comment #10) > I have in the past raised concern about removing set_window_position > and set_window_size from the client API we offer to end-users. The > WebDriver:SetWindowRect command is meant as a performance improvement > to reduce the number of driver requests, and unless youβre testing > the WebDriver protocol (as the WPT client does) it is more user > friendly to have the client API match what the user wants to do: > whilst it is possible to atomically reposition and resize the window > I donβt know how many users that is actually a useful primitive > for. Do you mean these functions are or could be useful primitives for users outside of Mozilla code base? If that is the case, the functions carry a warning message informing of being deprecated and direct the user to other functions. Could the messages be updated to mention a hard deadline for removal and be left untouched until then? As the final step, the definitions can be removed on the day after the deadline, no? > > Now I donβt care about this so strongly that I want to discourage > the attached patched, but at the same time I donβt think the WPT > WebDriver is a good source of inspiration here since it is geared > more towards testing the low-level protocol than it is towards > providing an idiomatic and user-friendly API. > > In any case, please feel free to move forward with the current plans.
Flags: needinfo?(ato)
Comment 12•7 years ago
|
||
(In reply to Venkatesh from comment #11) > Do you mean these functions are or could be useful primitives for > users outside of Mozilla code base? If that is the case, the > functions carry a warning message informing of being deprecated > and direct the user to other functions. Could the messages > be updated to mention a hard deadline for removal and be left > untouched until then? As the final step, the definitions can be > removed on the day after the deadline, no? I think probably the deprecation warnings have been there for long enough that it is fine to remove them now.
Flags: needinfo?(ato)
Comment 13•7 years ago
|
||
Comment on attachment 8965926 [details] [diff] [review] bug-1348145-attempt-02.patch Review of attachment 8965926 [details] [diff] [review]: ----------------------------------------------------------------- Removing feedback request given that Andreas gave his input.
Attachment #8965926 -
Flags: feedback?(hskupin)
Comment 14•7 years ago
|
||
Vankesh, if you can submit your patch via mozreview that would be fantastic. It needs a bit of setup, and you can find everything here: https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#mozreview-commits
Assignee | ||
Comment 15•7 years ago
|
||
setWindowPosition and getWindowPosition removed. Please comment.
Attachment #8965926 -
Attachment is obsolete: true
Attachment #8967237 -
Flags: review?(hskupin)
Assignee | ||
Comment 16•7 years ago
|
||
Hi Henrik, I get abort: authorisation failed on entering password. Resetting/changing password did not help too. I am trying `hg push -c 450059 review`. Where can I look to find out what is going on and how to fix?
Comment 17•7 years ago
|
||
(In reply to Venkatesh from comment #16) > Hi Henrik, I get abort: authorisation failed on entering password. > Resetting/changing password did not help too. I am trying `hg push -c > 450059 review`. Where can I look to find out what is going on and how to > fix? Did you setup the authentication key in Bugzilla, and added it to your hg config? That shouldn't force you to enter a password when trying to push.
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
Comment on attachment 8967237 [details] [diff] [review] bug-1348145-attempt-03.patch Looks like it worked now. mozreview is clearly better, so let me remove this obsolete attachment.
Attachment #8967237 -
Attachment is obsolete: true
Attachment #8967237 -
Flags: review?(hskupin)
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8967273 [details] Bug 1348145 - remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/235964/#review241934 Feedback wise this would be a f+ because that looks already kinda good. Thank you for that update! For a full review a couple of pieces are still missing. Please have a look at my inline review comments. Let me know if something is unclear. ::: commit-message-14217:1 (Diff revision 1) > +Bug 1348145 - remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. r?whimboo Note this only removes `getWindowPosition/setWindowPosition` yet. Are you planning to make the `getWindowSize/setWindowSize` removal part of this or another comment? You would have to update the commit message then. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:28 (Diff revision 1) > def test_get_types(self): > - position = self.marionette.get_window_position() > + position = self.marionette.window_rect > self.assertIn("x", position) > self.assertIn("y", position) > + self.assertIn("height", position) > + self.assertIn("width", position) You better may want to combine the two test classes for TestPosition and TestSize in that file. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:37 (Diff revision 1) > + self.assertIsInstance(position["width"], int) > > def test_set_types(self): > - for x, y in (["a", "b"], [1.2, 3.4], [True, False], [[], []], [{}, {}]): > + for x, y, h, w in (["a", "b", "h", "w"], [1.2, 3.4, 4.5, 5.6], > + [True, False, True, False], [[], [], [], []], > + [{}, {}, {}, {}]): For better readability please define that list already before the for loop, and then each entry in its own line. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:61 (Diff revision 1) > def test_move_to_new_position(self): > - old_position = self.marionette.get_window_position() > + old_position = self.marionette.window_rect > new_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10} > - self.marionette.set_window_position(new_position["x"], new_position["y"]) > + self.marionette.set_window_rect(new_position["x"], new_position["y"]) > self.assertNotEqual(old_position["x"], new_position["x"]) > self.assertNotEqual(old_position["y"], new_position["y"]) Also check the height and width that both haven't been changed. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:74 (Diff revision 1) > self.assertEqual(old_position["y"], new_position["y"]) > + self.assertEqual(old_position["height"], new_position["height"]) > + self.assertEqual(old_position["width"], new_position["width"]) > > def test_move_to_negative_coordinates(self): > + dims = self.marionette.window_rect Please continue to use `old_position` here. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:80 (Diff revision 1) > print("Current position: {}".format( > - self.marionette.get_window_position())) > - self.marionette.set_window_position(-8, -8) > - position = self.marionette.get_window_position() > - print("Position after requesting move to negative coordinates: {}".format(position)) > + dims["x"], dims["y"])) > + self.marionette.set_window_rect(-8, -8) > + position = self.marionette.window_rect > + print("Position after requesting move to negative coordinates: {}, {}" > + .format(position)) Only print x and y here.
Attachment #8967273 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 21•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #20) > Comment on attachment 8967273 [details] > ::: commit-message-14217:1 > (Diff revision 1) > > +Bug 1348145 - remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. r?whimboo > > Note this only removes `getWindowPosition/setWindowPosition` yet. Are you > planning to make the `getWindowSize/setWindowSize` removal part of this or > another comment? You would have to update the commit message then. > I was typing/copying from the title. I am thinking removing getWindowPosition/setWindowPosition can be worked on separately from getWindowSize/setWindowSize. Changed the commit message accordingly. Will it be OK? > ::: > testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py: > 28 > (Diff revision 1) > > def test_get_types(self): > > - position = self.marionette.get_window_position() > > + position = self.marionette.window_rect > > self.assertIn("x", position) > > self.assertIn("y", position) > > + self.assertIn("height", position) > > + self.assertIn("width", position) > > You better may want to combine the two test classes for TestPosition and > TestSize in that file. > Will it be OK to do this as a last step toward concluding work on this bug? > ::: > testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py: > 74 > (Diff revision 1) > > self.assertEqual(old_position["y"], new_position["y"]) > > + self.assertEqual(old_position["height"], new_position["height"]) > > + self.assertEqual(old_position["width"], new_position["width"]) > > > > def test_move_to_negative_coordinates(self): > > + dims = self.marionette.window_rect > > Please continue to use `old_position` here. Calling it `position` here so that the rest of the method can remain unedited.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8967273 -
Attachment is obsolete: true
Comment 23•7 years ago
|
||
(In reply to Venkatesh from comment #21) > > Note this only removes `getWindowPosition/setWindowPosition` yet. Are you > > planning to make the `getWindowSize/setWindowSize` removal part of this or > > another comment? You would have to update the commit message then. > > > I was typing/copying from the title. I am thinking removing > getWindowPosition/setWindowPosition can be worked on separately from > getWindowSize/setWindowSize. Changed the commit message accordingly. Will > it be OK? The patch itself is small enough to fit the removal of all those methods. It will also make it easier for you to combine the tests, without having an interim change. I would propose to do all in a single commit. Please note that for the commit you pushed earlier today the bug number in the commit message is wrong, and that it got attached to a different bug. Best always is to run `hg amend` to squash the latest changes in the previous patch. In your case run `hg histedit` to combine them. > > You better may want to combine the two test classes for TestPosition and > > TestSize in that file. > > Will it be OK to do this as a last step toward concluding work on this bug? The above proposal also answers this bug. > > Please continue to use `old_position` here. > > Calling it `position` here so that the rest of the method can remain > unedited. As best use what other tests are using. It makes it easier to understand.
Assignee | ||
Comment 24•7 years ago
|
||
A summary of what is in this patch: 1. remove setWindowSize/getWindowSize and all their usages 2. remove set_window_size/get_window_size and all their usages 3. bring the body of methods that had a large overlap of code into one (and renamed them to indicate this) 4. combined the two classes into one
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8967606 -
Attachment is obsolete: true
Attachment #8967606 -
Flags: review?(hskupin)
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8967606 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition. https://reviewboard.mozilla.org/r/236308/#review242658 I don't think that you wanted to discard this commit? ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:35 (Diff revision 1) > self.assertIsInstance(position["y"], int) > self.assertIsInstance(position["height"], int) > self.assertIsInstance(position["width"], int) > > def test_set_types(self): > - for x, y, h, w in (["a", "b", "h", "w"], [1.2, 3.4, 4.5, 5.6], > + illegal_rects = (["a", "b", "h", "w"], Please note the trailing space at the end. The linter will mark this as failure. Please fix it. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:64 (Diff revision 1) > def test_move_to_new_position(self): > old_position = self.marionette.window_rect > new_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10} > self.marionette.set_window_rect(new_position["x"], new_position["y"]) > self.assertNotEqual(old_position["x"], new_position["x"]) > self.assertNotEqual(old_position["y"], new_position["y"]) Oh, I didn't notice before... Why does this test not check for the new coordinates, but does a not equal check for the old position? It means we only test half of it. Please change it to `assertEqual()`.
Attachment #8967606 -
Flags: review?(hskupin)
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8967922 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/236620/#review242668 ::: testing/marionette/client/marionette_driver/marionette.py (Diff revision 1) > :param orientation: The orientation to lock the screen in. > """ > body = {"orientation": orientation} > self._send_message("setScreenOrientation", body) > > - @property The removal is fine, but getWindowPosition/setWindowPosition is no longer removed. I think you accidentally got rid of all your former commits? You can avoid that by checking the patches on mozreview before publishing. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:11 (Diff revision 1) > > from marionette_driver.errors import InvalidArgumentException > from marionette_harness import MarionetteTestCase > > > -class TestPosition(MarionetteTestCase): > +class TestPositionAndSize(MarionetteTestCase): Best call it `TestWindowRect`. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:14 (Diff revision 1) > > > -class TestPosition(MarionetteTestCase): > +class TestPositionAndSize(MarionetteTestCase): > > def setUp(self): > + super(MarionetteTestCase, self).setUp() When you use `super` then it has to the current class, but not the base class. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:16 (Diff revision 1) > -class TestPosition(MarionetteTestCase): > +class TestPositionAndSize(MarionetteTestCase): > > def setUp(self): > + super(MarionetteTestCase, self).setUp() > + > MarionetteTestCase.setUp(self) You already called `setUp` of the base class above. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:17 (Diff revision 1) > > def setUp(self): > + super(MarionetteTestCase, self).setUp() > + > MarionetteTestCase.setUp(self) > self.original_position = self.marionette.window_rect This should be `original_rect` and at the end of `setUp`. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:44 (Diff revision 1) > self.marionette.set_window_rect(x, y, height, width) > + is_fullscreen = self.marionette.execute_script("return document.fullscreenElement;", sandbox=None) > + if is_fullscreen: > + self.marionette.fullscreen() > MarionetteTestCase.tearDown(self) > + super(MarionetteTestCase, self).tearDown() Same here for the usage of `super`, and the double call to `tearDown`. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:73 (Diff revision 1) > def test_setting_window_rect_with_nulls_errors(self): > with self.assertRaises(InvalidArgumentException): > self.marionette.set_window_rect(height=None, width=None, > x=None, y=None) > > - def test_set_position_with_rect(self): > + def test_set_position_and_size_with_rect(self): Make sure that we test: 1) Set position only 2) Set size only 3) Set both position and size
Attachment #8967922 -
Flags: review?(hskupin) → review-
Comment hidden (mozreview-request) |
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8967922 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/236620/#review242886 Code analysis found 3 defects in this patch: - 3 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:118 (Diff revision 2) > + self.assertNotEqual(old["x"], new["x"]) > + self.assertNotEqual(old["y"], new["y"]) > + > + def test_move_to_new_size(self): > + old = self.marionette.window_rect > + new = {"height": old["height"] + 10, "widht": old["width"]) Error: Invalid syntax [py3: is-parseable] ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:118 (Diff revision 2) > + self.assertNotEqual(old["x"], new["x"]) > + self.assertNotEqual(old["y"], new["y"]) > + > + def test_move_to_new_size(self): > + old = self.marionette.window_rect > + new = {"height": old["height"] + 10, "widht": old["width"]) Error: Invalid syntax [py3: is-parseable] ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:118 (Diff revision 2) > + self.assertNotEqual(old["x"], new["x"]) > + self.assertNotEqual(old["y"], new["y"]) > + > + def test_move_to_new_size(self): > + old = self.marionette.window_rect > + new = {"height": old["height"] + 10, "widht": old["width"]) Error: Invalid syntax [py2: is-parseable]
Comment hidden (mozreview-request) |
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8967922 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/236620/#review242900 Code analysis found 3 defects in this patch: - 3 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:118 (Diff revision 3) > + self.assertNotEqual(old["x"], new["x"]) > + self.assertNotEqual(old["y"], new["y"]) > + > + def test_move_to_new_size(self): > + old = self.marionette.window_rect > + new = {"height": old["height"] + 10, "width": old["width"]) Error: Invalid syntax [py2: is-parseable] ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:118 (Diff revision 3) > + self.assertNotEqual(old["x"], new["x"]) > + self.assertNotEqual(old["y"], new["y"]) > + > + def test_move_to_new_size(self): > + old = self.marionette.window_rect > + new = {"height": old["height"] + 10, "width": old["width"]) Error: Invalid syntax [py2: is-parseable] ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:118 (Diff revision 3) > + self.assertNotEqual(old["x"], new["x"]) > + self.assertNotEqual(old["y"], new["y"]) > + > + def test_move_to_new_size(self): > + old = self.marionette.window_rect > + new = {"height": old["height"] + 10, "width": old["width"]) Error: Invalid syntax [py3: is-parseable]
Comment hidden (mozreview-request) |
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8967922 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/236620/#review243334 This patch look way better an includes everything we need. Thanks! Nevertheless I had to point out a couple of comments. Please have a look at those, and make sure to `hg commit --amend` the changes before pushing. Also please run all the tests, and also the linter (mach lint testing/marionette). ::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:76 (Diff revision 4) > # Ensure the window is large enough to show the button. > - self.marionette.set_window_size(1200, 1200) > + self.marionette.set_window_rect(height=1200, width=1200) > self.assertEqual(wrapper.value_of_css_property('visibility'), 'visible') > > # Set the window small enough that the button disappears. > - self.marionette.set_window_size(335, 335) > + self.marionette.set_window_rect(height=335, width=335) Good catch! ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:16 (Diff revision 4) > -class TestPosition(MarionetteTestCase): > +class TestWindowRect(MarionetteTestCase): > > def setUp(self): > - MarionetteTestCase.setUp(self) > - self.original_position = self.marionette.get_window_position() > + self.original_rect = self.marionette.window_rect > + > + super(TestWindowRect, self).setUp() You should call the super __init__ method first before doing any special initialization for that class. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:42 (Diff revision 4) > - self.marionette.set_window_position(x, y) > - MarionetteTestCase.tearDown(self) > + height, width = self.original_rect["height"], self.original_rect["width"] > + self.marionette.set_window_rect(x, y, height, width) > + is_fullscreen = self.marionette.execute_script("return document.fullscreenElement;", sandbox=None) > + if is_fullscreen: > + self.marionette.fullscreen() > + super(TestWindowRect, self).tearDown() nit: please add a blank line before. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:45 (Diff revision 4) > + if is_fullscreen: > + self.marionette.fullscreen() > + super(TestWindowRect, self).tearDown() > > def test_get_types(self): > - position = self.marionette.get_window_position() > + position = self.marionette.window_rect it's not the position only anymore. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:56 (Diff revision 4) > self.assertIsInstance(position["y"], int) > + self.assertIsInstance(position["height"], int) > + self.assertIsInstance(position["width"], int) > > def test_set_types(self): > - for x, y in (["a", "b"], [1.2, 3.4], [True, False], [[], []], [{}, {}]): > + illegal_rects = (["a", "b", "h", "w"], nit: there is still the trailing white-space. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:75 (Diff revision 4) > > - def test_set_position_with_rect(self): > + def test_set_position(self): > old_position = self.marionette.window_rect > wanted_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10} > > - new_position = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"]) > + new = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"]) You can keep `new_position` here. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:78 (Diff revision 4) > wanted_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10} > > - new_position = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"]) > + new = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"]) > + > + self.assertNotEqual(old_position["x"], new["x"]) > + self.assertNotEqual(old_position["y"], new["y"]) You didn't reply to one of my last review comments here... Why not checking for the new position? We would miss half of the test here. Or would that not work? Also make sure to add the expected state as second argument. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:80 (Diff revision 4) > - new_position = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"]) > + new = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"]) > + > + self.assertNotEqual(old_position["x"], new["x"]) > + self.assertNotEqual(old_position["y"], new["y"]) > + > + def test_set_size(self): Note, for an easier reviewing it would have been good to not move test methods around in that file at the same time when you make changes. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:84 (Diff revision 4) > + > + def test_set_size(self): > + old_size = self.marionette.window_rect > + wanted_size = {"height": old_size["height"] - 50, "width": old_size["width"] - 50} > > - self.assertNotEqual(old_position["x"], new_position["x"]) > + new = self.marionette.set_window_rect(height=wanted_size["height"], weight=wanted_size["width"]) `new_size` please ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:91 (Diff revision 4) > + self.assertEqual(new["width"], wanted_size["width"], > + "New width is {0} but should be {1}".format(new["width"], wanted_size["width"])) > + self.assertEqual(new["height"], wanted_size["height"], > + "New height is {0} but should be {1}".format(new["height"], wanted_dize["height"])) > + > + def test_set_position_and_size_with_rect(self): Remove the `with_rect` suffix from the test name. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:92 (Diff revision 4) > + "New width is {0} but should be {1}".format(new["width"], wanted_size["width"])) > + self.assertEqual(new["height"], wanted_size["height"], > + "New height is {0} but should be {1}".format(new["height"], wanted_dize["height"])) > + > + def test_set_position_and_size_with_rect(self): > + old_position = self.marionette.window_rect `old_rect` given that you cover position and size. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:95 (Diff revision 4) > + > + def test_set_position_and_size_with_rect(self): > + old_position = self.marionette.window_rect > + wanted_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10} > + width = old_position["width"] - 50 > + height = old_position["height"] - 50 Include those two lines into the `wanted_rect`, similar to x and y. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:101 (Diff revision 4) > + > + new = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"], > + width=width, height=height) > + > + self.assertNotEqual(old_position["x"], new["x"]) > + self.assertNotEqual(old_position["y"], new["y"]) the expected value has to be the second argument. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:107 (Diff revision 4) > + self.assertEqual(new["width"], width, > + "New width is {0} but should be {1}".format(new["width"], width)) > + self.assertEqual(new["height"], height, > + "New height is {0} but should be {1}".format(new["height"], height)) > > def test_move_to_new_position(self): What's the difference to `test_set_position`? ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:116 (Diff revision 4) > - self.assertNotEqual(old_position["x"], new_position["x"]) > - self.assertNotEqual(old_position["y"], new_position["y"]) > + self.marionette.set_window_rect(x=new["x"], y=new["y"]) > + > + self.assertNotEqual(old["x"], new["x"]) > + self.assertNotEqual(old["y"], new["y"]) > + > + def test_move_to_new_size(self): What's the difference to `test_set_size`? ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:126 (Diff revision 4) > + actual = self.marionette.window_rect > + > + self.assertEqual(actual["width"], new["width"]) > + self.assertEqual(actual["height"], new["height"]) > + > + def test_move_to_new_position_and_size(self): What's the difference to `test_set_position_and_size_with_rect`? ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:148 (Diff revision 4) > - self.marionette.set_window_position(old_position["x"], old_position["y"]) > - new_position = self.marionette.get_window_position() > - self.assertEqual(old_position["x"], new_position["x"]) > - self.assertEqual(old_position["y"], new_position["y"]) > + self.marionette.set_window_rect(x=old["x"], y=old["y"]) > + > + new = self.marionette.window_rect > + > + self.assertEqual(old["x"], new["x"]) > + self.assertEqual(old["y"], new["y"]) The expected value has to be the second argument. The same applies to all the following asserts.
Attachment #8967922 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 34•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #33) > This patch look way better an includes everything we need. Thanks! > Nevertheless I had to point out a couple of comments. Please have a look at > those, and make sure to `hg commit --amend` the changes before pushing. Also > please run all the tests, and also the linter (mach lint testing/marionette). > I got the ./mach lint ./path/to/file[s]. How do I run tests? > Good catch! > Thank you :) I am not clear on whether to move this https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executorselenium.py#341 to set_window_rect as well. Because there is no marionette imported at the top. > > You didn't reply to one of my last review comments here... > > Why not checking for the new position? We would miss half of the test here. > Or would that not work? > Sorry about that, I should have missed. Which previous comment have I missed? And, is new position a reference to the variable name? Or, I am not clear on what you are asking here. > Also make sure to add the expected state as second argument. > Doing this everywhere. > Note, for an easier reviewing it would have been good to not move test > methods around in that file at the same time when you make changes. > Agreed. > > def test_move_to_new_position(self): > > What's the difference to `test_set_position`? > There isn't any. Something I missed. Removed the method. > > + def test_move_to_new_size(self): > > What's the difference to `test_set_size`? > Removed. There isn't any. > > + def test_move_to_new_position_and_size(self): > > What's the difference to `test_set_position_and_size_with_rect`? > Removed as well. > > + self.assertEqual(old["x"], new["x"]) > > + self.assertEqual(old["y"], new["y"]) > > The expected value has to be the second argument. The same applies to all > the following asserts. Doing this everywhere. Pending these, I have submitted to review.
Flags: needinfo?(hskupin)
Comment hidden (mozreview-request) |
Comment 36•7 years ago
|
||
You can find more information about running tests here: https://firefox-source-docs.mozilla.org/testing/marionette/marionette/Testing.html And more general information on contributing to Marionette: https://firefox-source-docs.mozilla.org/testing/marionette/marionette/Contributing.html
Comment 37•7 years ago
|
||
(In reply to Venkatesh from comment #34) > I am not clear on whether to move this > https://searchfox.org/mozilla-central/source/testing/web-platform/tests/ > tools/wptrunner/wptrunner/executors/executorselenium.py#341 to > set_window_rect as well. Because there is no marionette imported at the top. No, this is using the webdriver client. See the appropriate link in comment 6. There is nothing you have to care about. > > Why not checking for the new position? We would miss half of the test here. > > Or would that not work? > > Sorry about that, I should have missed. Which previous comment have I > missed? And, is new position a reference to the variable name? Or, I am > not clear on what you are asking here. Maybe you missed it because older patches have been marked as obsolete, and I was commenting on a discarded commit. Anyway please have a look at the correct checks. And yes, with `new_position` I was referring to the variable name.
Flags: needinfo?(hskupin)
Assignee | ||
Comment 38•7 years ago
|
||
(In reply to Andreas Tolfsen βΉ:atoβΊ from comment #36) > You can find more information about running tests here: > https://firefox-source-docs.mozilla.org/testing/marionette/marionette/ > Testing.html > > And more general information on contributing to Marionette: > https://firefox-source-docs.mozilla.org/testing/marionette/marionette/ > Contributing.html I am failing at the setup step `pip install -r testing/config/marionette_requirements.txt` listed at https://firefox-source-docs.mozilla.org/testing/marionette/marionette/Testing.html with Invalid requirement: '../tools/mozterm' It looks like a path. Does it exist? output of `python -m site`: sys.path = [ '/home/venkatesh/src/mozilla-unified', '/usr/lib/python2.7', '/usr/lib/python2.7/plat-x86_64-linux-gnu', '/usr/lib/python2.7/lib-tk', '/usr/lib/python2.7/lib-old', '/usr/lib/python2.7/lib-dynload', '/home/venkatesh/.local/lib/python2.7/site-packages', '/usr/local/lib/python2.7/dist-packages', '/usr/lib/python2.7/dist-packages', '/usr/lib/python2.7/dist-packages/PILcompat', '/usr/lib/python2.7/dist-packages/gtk-2.0', '/usr/lib/pymodules/python2.7', '/usr/lib/python2.7/dist-packages/wx-3.0-gtk3', ] USER_BASE: '/home/venkatesh/.local' (exists) USER_SITE: '/home/venkatesh/.local/lib/python2.7/site-packages' (exists) ENABLE_USER_SITE: True How do I solve this?
Flags: needinfo?(ato)
Updated•7 years ago
|
Flags: needinfo?(ato) → needinfo?(hskupin)
Comment 39•7 years ago
|
||
(In reply to Venkatesh from comment #38) > I am failing at the setup step `pip install -r > testing/config/marionette_requirements.txt` listed at > https://firefox-source-docs.mozilla.org/testing/marionette/marionette/ > Testing.html with This is actually wrong. This part of the documentation relates to the test archives, which you would have to download from a finished build. This is not something what you want to do. So check the paragraph above regarding `mach`. This is the right tool to run the Marionette and other tests.
Flags: needinfo?(hskupin)
Comment 40•7 years ago
|
||
A side-note: Is it possible the documentation can be misunderstood here? I think possibly it isnβt clear to contributors if they want to run tests form a βdownloaded test archiveβ or not.
Comment 41•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967922 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/236620/#review243334 > You didn't reply to one of my last review comments here... > > Why not checking for the new position? We would miss half of the test here. Or would that not work? > > Also make sure to add the expected state as second argument. This has still not resolved. Please check the open issues and if you responded to all of them before publishing a next version of the patch. It helps to speed-up the process. In case of questions just add a comment for the issue in mozreview. > What's the difference to `test_set_position_and_size_with_rect`? Still identical to `test_set_position_and_size`, right?
Comment 42•7 years ago
|
||
mozreview-review |
Comment on attachment 8967922 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/236620/#review244610 ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:79 (Diff revisions 4 - 5) > def test_set_position(self): > old_position = self.marionette.window_rect > wanted_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10} > > - new = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"]) > + self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"]) > + new_position = self.marionette.window_rect Please do not use the getter here, but keep what `set_window_rect` returns. What you then could do is to assert that the return value and what `window_rec` returns is equal. That would actually be a nice addition. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:89 (Diff revisions 4 - 5) > def test_set_size(self): > old_size = self.marionette.window_rect > wanted_size = {"height": old_size["height"] - 50, "width": old_size["width"] - 50} > > - new = self.marionette.set_window_rect(height=wanted_size["height"], weight=wanted_size["width"]) > - > + self.marionette.set_window_rect(height=wanted_size["height"], weight=wanted_size["width"]) > + new_size = self.marionette.window_rect Same as above. Use the return value from `set_window_rect`. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:99 (Diff revisions 4 - 5) > - "New width is {0} but should be {1}".format(new["width"], width)) > - self.assertEqual(new["height"], height, > - "New height is {0} but should be {1}".format(new["height"], height)) > - > - def test_move_to_new_position(self): > - old = self.marionette.window_rect > + "New height is {0} but should be {1}".format(new_size["height"], wanted_dize["height"])) > + > + def test_set_position_and_size(self): > + old_rect = self.marionette.window_rect > + wanted_rect = {"x": old_rect["x"] + 10, "y": old_rect["y"] + 10, > + "width": old_rect["width"] - 50, "height": old_rect["height"] - 50} The indentation here is wrong and the linter should complain about it. Start directly under "x". ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:103 (Diff revisions 4 - 5) > - def test_move_to_new_position(self): > - old = self.marionette.window_rect > - new = {"x": old["x"] + 10, "y": old["y"]} > - > - self.marionette.set_window_rect(x=new["x"], y=new["y"]) > - > + wanted_rect = {"x": old_rect["x"] + 10, "y": old_rect["y"] + 10, > + "width": old_rect["width"] - 50, "height": old_rect["height"] - 50} > + > + self.marionette.set_window_rect(x=wanted_rect["x"], y=wanted_rect["y"], > + width=wanted_rect["width"], height=wanted_rect["height"]) > + new_rect = self.marionette.window_rect Same as above for using the return value of "set_window_rect". ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:106 (Diff revisions 4 - 5) > - > - self.marionette.set_window_rect(x=new["x"], y=new["y"]) > - > - self.assertNotEqual(old["x"], new["x"]) > - self.assertNotEqual(old["y"], new["y"]) > - > + self.marionette.set_window_rect(x=wanted_rect["x"], y=wanted_rect["y"], > + width=wanted_rect["width"], height=wanted_rect["height"]) > + new_rect = self.marionette.window_rect > + > + self.assertNotEqual(new_rect["x"], old_rect["x"]) > + self.assertNotEqual(new_rect["y"], old_rect["y"]) Still why not equal and not equal? ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:127 (Diff revisions 4 - 5) > - self.assertNotEqual(old["y"], new["y"]) > + self.assertNotEqual(new["y"], old["y"]) > > self.assertEqual(actual["width"], new["width"]) > self.assertEqual(actual["height"], new["height"]) > > def test_move_to_existing_position(self): To better understand this test and the following, would you mind to update the title from `existing` to `current`? It's a bit unclear what existing means here. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:223 (Diff revisions 4 - 5) > self.assertGreaterEqual(new["height"], self.max["height"]) > > def test_resize_to_available_screen_size(self): > - result = self.marionette.set_window_rect(width=self.max['width'], > + self.marionette.set_window_rect(width=self.max['width'], > - height=self.max["height"]) > + height=self.max["height"]) > + result = self.marionette.window_rect Same as above to use the return value. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:232 (Diff revisions 4 - 5) > > def test_resize_while_fullscreen(self): > self.marionette.fullscreen() > - result = self.marionette.set_window_rect(width=self.max["width"] - 100, > + self.marionette.set_window_rect(width=self.max["width"] - 100, > - height=self.max["height"] - 100) > + height=self.max["height"] - 100) > + result = self.marionette.window_rect Here too.
Attachment #8967922 -
Flags: review?(hskupin) → review-
Comment 43•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967922 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/236620/#review242886 > Error: Invalid syntax [py3: is-parseable] Can you please close those bot issues once you addressed them all? I'm not allowed to do so. Thanks.
Comment 44•7 years ago
|
||
(In reply to Andreas Tolfsen βΉ:atoβΊ from comment #40) > A side-note: Is it possible the documentation can be misunderstood > here? I think possibly it isnβt clear to contributors if they want > to run tests form a βdownloaded test archiveβ or not. Given that nowadays you usually run via `mach` and only use the test archive on one-click loaners, we should get rid of this section. I should have this covered that part in the other doc for taskcluster already.
Assignee | ||
Comment 45•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967922 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/236620/#review243334 > This has still not resolved. Please check the open issues and if you responded to all of them before publishing a next version of the patch. It helps to speed-up the process. In case of questions just add a comment for the issue in mozreview. Done, now testing against wanted_position as well as expected_position as returned by the getter. > Still identical to `test_set_position_and_size`, right? Yes, simply dropped the _with_rect suffix.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 47•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967922 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/236620/#review243334 > Yes, simply dropped the _with_rect suffix. Sorry I missed the previous comment. Are you suggesting that test_setting_window_rect_with_nulls_errors and test_set_position_and_size be made into one method?
Comment hidden (mozreview-request) |
Comment 49•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967922 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/236620/#review243334 > Sorry I missed the previous comment. Are you suggesting that test_setting_window_rect_with_nulls_errors and test_set_position_and_size be made into one method? No but `test_move_to_new_position_and_size` and `test_set_position_and_size` looks the same.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 51•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #49) > Comment on attachment 8967922 [details] > Bug 1348145 - Remove getWindowPosition/setWindowPosition and > getWindowSize/setWindowSize. > > https://reviewboard.mozilla.org/r/236620/#review243334 > > > Sorry I missed the previous comment. Are you suggesting that test_setting_window_rect_with_nulls_errors and test_set_position_and_size be made into one method? > > No but `test_move_to_new_position_and_size` and `test_set_position_and_size` > looks the same. I removed test_move_to_new_position_and_size and submitted for review. Is it alright? What else is left to conclude work on this bug?
Comment 53•6 years ago
|
||
mozreview-review |
Comment on attachment 8967922 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/236620/#review247256 Sorry that this review has been taken a bit longer. But there have been public holidays and some time off. As it looks like we are not that far from having the final code ready. Most likely only one remaining update of your patch is necessary. Once done I will submit it to try. Thank you! ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:126 (Diff revisions 5 - 8) > - self.assertEqual(actual["width"], new["width"]) > - self.assertEqual(actual["height"], new["height"]) > - > - def test_move_to_existing_position(self): > old = self.marionette.window_rect > self.marionette.set_window_rect(x=old["x"], y=old["y"]) Please use the return value from set_window_rect similar to above. Same applies to the next two methods too. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:209 (Diff revisions 5 - 8) > > - > def test_resize_larger_than_screen(self): > - self.marionette.set_window_rect( > + new = self.marionette.set_window_rect( > self.max["width"] * 2, self.max["height"] * 2) > - new = self.marionette.window_rect > + actual = self.marionette.window_rect Try to keep the naming schema between tests. Above you used `new_rect` and `expected_rect` which I find more descriptive. This also applies to the next three methods.
Attachment #8967922 -
Flags: review?(hskupin) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8967922 -
Attachment is obsolete: true
Assignee | ||
Comment 55•6 years ago
|
||
./mach marionette test -z fails with 120 second server timeout. So I tried merge and commit. Didn't work. Then I backed out. hg histedit -o aborts saying cannot edit history that contains merge. So this review submission is the edits just to test_window_rect.py. How do I generate the complete patch?
Comment 56•6 years ago
|
||
(In reply to Venkatesh from comment #55) > ./mach marionette test -z fails with 120 second server timeout. So I tried Did you pull from mozilla-central and missed to run "mach build" afterward? This could indicate a problem like that. If not please reach out to us on IRC and we can figure it out. > merge and commit. Didn't work. Then I backed out. hg histedit -o aborts > saying cannot edit history that contains merge. So this review submission > is the edits just to test_window_rect.py. How do I generate the complete > patch? Given that I don't know which exact commands you run, I cannot really help. Generally when you are working on a bookmark no backout is necessary. Maybe that confused mercurial, and now you are blocked on the merge commit. Best solution here is most likely to create a new bookmark, import the patch from the first attachment, and then apply your fix-up on top of it. Running "hg push review" afterward should update the mozreview patch on this bug.
Comment 57•6 years ago
|
||
Note, given that you closed the former mozreview request, reopen it please in mozreview. And first attachment from my last comment actually applies to this code.
Comment 58•6 years ago
|
||
mozreview-review |
Comment on attachment 8973373 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/241830/#review247836
Attachment #8973373 -
Flags: review?(hskupin)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8973373 -
Attachment is obsolete: true
Comment 60•6 years ago
|
||
mozreview-review |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review248492 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: testing/marionette/client/marionette_driver/marionette.py:15 (Diff revision 1) > import os > import socket > import sys > import time > import traceback > import warnings Error: 'warnings' imported but unused [flake8: F401]
Comment hidden (mozreview-request) |
Comment 62•6 years ago
|
||
Venkatesh, I don't know why mozreview always creates a complete new review request / attachment for every time you upload something. For reviewers it means they have to re-review the whole patch because no interdiff is available. I will do that now, but for the next bug you want to work on, we should make sure that this doesn't happen anymore. Thanks!
Comment 63•6 years ago
|
||
mozreview-review |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review248950 There are still a couple of things to fix. Beside that I will already trigger a try build so that we can see if there is something else which is failing and needs an update. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:135 (Diff revision 2) > - self.assertNotEqual(old_position["x"], new_position["x"]) > - self.assertNotEqual(old_position["y"], new_position["y"]) > + self.assertEqual(new_position["x"], old_position["x"]) > + self.assertEqual(new_position["y"], old_position["y"]) > > - def test_move_to_new_position(self): > - old_position = self.marionette.get_window_position() > - new_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10} > + def test_move_to_current_size(self): > + old_size = self.marionette.window_rect > + self.marionette.set_window_rect(height=old_size["height"], width=old_size["width"]) Similar to other tests you still missed to add the use of the return value here as new_size. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:140 (Diff revision 2) > - new_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10} > - self.marionette.set_window_position(new_position["x"], new_position["y"]) > - self.assertNotEqual(old_position["x"], new_position["x"]) > - self.assertNotEqual(old_position["y"], new_position["y"]) > - > - def test_move_to_existing_position(self): > + self.marionette.set_window_rect(height=old_size["height"], width=old_size["width"]) > + > + new_size = self.marionette.window_rect > + > + self.assertEqual(new_size["height"], old_size["height"]) > + self.assertEuqal(new_size["width"], old_size["width"]) This is a typing failure which should have marked the test as failed. Did you run the test locally? ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:144 (Diff revision 2) > - > - def test_move_to_existing_position(self): > - old_position = self.marionette.get_window_position() > - self.marionette.set_window_position(old_position["x"], old_position["y"]) > - new_position = self.marionette.get_window_position() > - self.assertEqual(old_position["x"], new_position["x"]) > + self.assertEqual(new_size["height"], old_size["height"]) > + self.assertEuqal(new_size["width"], old_size["width"]) > + > + def test_move_to_current_position_and_size(self): > + old_position_and_size = self.marionette.window_rect > + self.marionette.set_window_rect(old_position_and_size["x"], old_position_and_size["y"], Similar for the return value here. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:215 (Diff revision 2) > > # in X the window size may be greater than the bounds of the screen > - self.assertGreaterEqual(new["width"], self.max["width"]) > - self.assertGreaterEqual(new["height"], self.max["height"]) > + self.assertGreaterEqual(new_size["width"], self.max["width"]) > + self.assertGreaterEqual(new_size["height"], self.max["height"]) > + self.assertEqual(new_size["width"], actual_size["width"]) > + self.assertEqual(new_size["height"], actual_size["width"]) Given the comment from above we might not be able to do an equal check? Did you test this? ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:220 (Diff revision 2) > + self.assertEqual(new_size["height"], actual_size["width"]) > > def test_resize_to_available_screen_size(self): > - result = self.marionette.set_window_rect(width=self.max['width'], > + expected_size = self.marionette.set_window_rect(width=self.max["width"], > - height=self.max["height"]) > + height=self.max["height"]) > - self.assertEqual(result["width"], self.max["width"]) > + result_size = self.marionette.window_rect Please try to keep the naming schema across tests for the variables. That makes it easier to read and to understand what we test. So far I have seen 3 variations. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:225 (Diff revision 2) > - self.assertEqual(result["width"], self.max["width"]) > - self.assertEqual(result["height"], self.max["height"]) > + result_size = self.marionette.window_rect > + > + self.assertEqual(result_size["width"], self.max["width"]) > + self.assertEqual(result_size["height"], self.max["height"]) > + self.assertEqual(result_size["width"], self.expected_size["width"]) > + self.assertEqual(result_size["height"], self.expected_size["height"]) You should better compare expected_size with max, because that's what we interested in. Keep in mind that if result_size is wrong, you don't know where we fail. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:238 (Diff revision 2) > self.assertTrue(self.marionette.execute_script("return window.fullscreenElement == null", > sandbox=None)) > - self.assertEqual(result["width"], self.max["width"] - 100) > - self.assertEqual(result["height"], self.max["height"] - 100) > + self.assertEqual(result_size["width"], self.max["width"] - 100) > + self.assertEqual(result_size["height"], self.max["height"] - 100) > + self.assertEqual(result_size["width"], self.expected_size["width"]) > + self.assertEqual(result_size["height"], self.expected_size["height"]) Same as above.
Attachment #8974311 -
Flags: review?(hskupin) → review-
Comment 64•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review248492 > Error: 'warnings' imported but unused [flake8: F401] Can you please close this issue? I don't have permissions to do so.
Comment 65•6 years ago
|
||
The try results can be found here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8acade37095c25e874609bf05165774635f7eb3a
Assignee | ||
Comment 66•6 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #62) > Venkatesh, I don't know why mozreview always creates a complete new review > request / attachment for every time you upload something. For reviewers it > means they have to re-review the whole patch because no interdiff is > available. I will do that now, but for the next bug you want to work on, we > should make sure that this doesn't happen anymore. Thanks! I did not realise it was happening. Sorry for that. What can I do to help?
Comment hidden (mozreview-request) |
Comment 68•6 years ago
|
||
(In reply to Venkatesh from comment #66) > I did not realise it was happening. Sorry for that. What can I do to help? The way you did the last update of the patch was fine. Whatever was the difference to previous ones... As you can see in the comment before there is a link to https://reviewboard.mozilla.org/r/242630/diff/2-3/ which is the interdiff I usually have a look at. Generally make sure to keep the mozreview id in the commit message when updating/rebasing/histediting commits. And more importantly push to mozreview and publish from there instead from the command line. That way you can check the patch again if everything is alright before publishing the changes.
Comment 69•6 years ago
|
||
mozreview-review |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review249278 ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:208 (Diff revisions 2 - 3) > > # in X the window size may be greater than the bounds of the screen > - self.assertGreaterEqual(new_size["width"], self.max["width"]) > - self.assertGreaterEqual(new_size["height"], self.max["height"]) > - self.assertEqual(new_size["width"], actual_size["width"]) > - self.assertEqual(new_size["height"], actual_size["width"]) > + self.assertGreaterEqual(self.max["width"], new_size["width"]) > + self.assertGreaterEqual(self.max["height"], new_size["height"]) > + self.assertGreaterEqual(self.max["width"], actual_size["width"]) > + self.assertGreaterEqual(self.max["height"], actual_size["height"]) Why are you flipping the arguments of the assert method here? As mentioned in the comment above the window size might be larger than the max and not the other way around.
Attachment #8974311 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 70•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review249278 > Why are you flipping the arguments of the assert method here? As mentioned in the comment above the window size might be larger than the max and not the other way around. From the ./mach marionette test -z I run, I get: new_size == actual_size -> PASS new_size >= max -> PASS So, max >= new_size is a fail. For example, max[width] == 1356 and new_size[width] == 1366. My screen resolution is 1366x768 (running Debian unstable, GNOME, updated/upgraded once every week to ten days). I think what I am asking is, is 'window size' in the comment mapping to new_size/actual_size and 'bounds of the screen' to max? Or should the mapping be flipped around?
Assignee | ||
Comment 71•6 years ago
|
||
(In reply to Venkatesh from comment #70) > So, max >= new_size is a fail. For example, max[width] == 1356 and > new_size[width] == 1366. My screen resolution is 1366x768 (running Debian > unstable, GNOME, updated/upgraded once every week to ten days). Correction: I meant to say max[width] == 1366, new_size[width] == 1356.
Comment 72•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review249278 > From the ./mach marionette test -z I run, I get: > > new_size == actual_size -> PASS > new_size >= max -> PASS > > So, max >= new_size is a fail. For example, max[width] == 1356 and new_size[width] == 1366. My screen resolution is 1366x768 (running Debian unstable, GNOME, updated/upgraded once every week to ten days). > > I think what I am asking is, is 'window size' in the comment mapping to new_size/actual_size and 'bounds of the screen' to max? Or should the mapping be flipped around? So what you also might want to try is to run this test not in headless mode. You probably will get other results. What I basically wonder is why was the assertion passing before with the values exchanged. Please run the original test without your modifications and check what that reports, and if it passes.
Assignee | ||
Comment 73•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review249278 > So what you also might want to try is to run this test not in headless mode. You probably will get other results. > > What I basically wonder is why was the assertion passing before with the values exchanged. Please run the original test without your modifications and check what that reports, and if it passes. Below is a paste of failure message for height. FAILED TESTS ------- 7:04.33 INFO test_window_rect.py test_window_rect.TestWindowRect.test_resize_larger_than_screen 7:04.33 SUITE_END marionette-test ~~~~~~~~~~~~~~~ Ran 902 checks (902 tests) Expected results: 853 Skipped: 48 tests Unexpected results: 1 test: 1 (1 fail) Unexpected Results ------------------ FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen - AssertionError: 758 not greater than or equal to 768 Traceback (most recent call last): File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 159, in run testMethod() File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py", line 206, in test_resize_larger_than_screen self.assertGreaterEqual(actual_size["height"], self.max["height"]) width comparison fails similarly as well. new_size == actual_size passes.
Assignee | ||
Comment 74•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review249278 > Below is a paste of failure message for height. > > FAILED TESTS > ------- > 7:04.33 INFO test_window_rect.py test_window_rect.TestWindowRect.test_resize_larger_than_screen > 7:04.33 SUITE_END > > marionette-test > ~~~~~~~~~~~~~~~ > Ran 902 checks (902 tests) > Expected results: 853 > Skipped: 48 tests > Unexpected results: 1 > test: 1 (1 fail) > > Unexpected Results > ------------------ > FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen - AssertionError: 758 not greater than or equal to 768 > Traceback (most recent call last): > File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 159, in run > testMethod() > File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py", line 206, in test_resize_larger_than_screen > self.assertGreaterEqual(actual_size["height"], self.max["height"]) > > > width comparison fails similarly as well. > > new_size == actual_size passes. Running not in headless has more failures than running in headless. FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_move_to_negative_coordinates - AssertionError: 27 not less than or equal to 0 FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen - AssertionError: 704 not greater than or equal to 741 FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_set_position - AssertionError: 0 != 10 Traceback (most recent call last): FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_maximize.py TestWindowMaximize.test_maximize - AssertionError: Window height is not within 22 px of availHeight: current height 704 should be greater than or equal to max height 719 FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_maximize.py TestWindowMaximize.test_maximize_twice_is_idempotent - AssertionError: Window height is not within 22 px of availHeight: current height 704 should be greater than or equal to max height 719 FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_maximize.py TestWindowMaximize.test_stress - AssertionError: Window height is not within 22 px of availHeight: current height 704 should be greater than or equal to max height 719
Comment 75•6 years ago
|
||
Andreas, given that you wrote all the window resize tests, could you have a look at the last comment from Venkatesh? Flipping the max and expected looks strange, but also that tests fail without the modifications.
Flags: needinfo?(ato)
Comment 76•6 years ago
|
||
Which OS are you running the tests on? Window resizing is to a large degree dependent on the window manager (WM) and the desktop environment (DE) that you use. Some things which may be possible in the DE we use on try might not be possible to you locally. The characteristics of your environment and the requirement to have tests passing in other environments unlike your own restrict how the tests can be written. In some cases the assertions have to be relaxed to encompass a larger range, and in other cases it makes more sense to use two different assertion values depending on the system (macOS vs. Linux vs. Windows for example). When you run the tests on Linux, you need to make sure you run them in an xvfb with a WM that supports the same capabilities we use on try. I donβt know enough about our configuration on try to say what that is, but I know it uses xvfb and some kind of simple WM. The headless tests should pass regardless, so I would focus my time on fixing those problems first. Then for the other configurations I would use try to make sure the tests pass.
Flags: needinfo?(ato)
Assignee | ||
Comment 77•6 years ago
|
||
How can I run the tests on try server myself? I am thinking it will be better to submit next review after passing the tests on try server.
Flags: needinfo?(hskupin)
Comment 78•6 years ago
|
||
We discussed on IRC. So I will trigger it for you once the changes have been uploaded.
Flags: needinfo?(hskupin)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 80•6 years ago
|
||
I uploaded a patch/diff to the review board sometime in the last week. Did it pass the tests on the try server? How or where can I see them results?
Flags: needinfo?(hskupin)
Comment 81•6 years ago
|
||
I was out the last days but will have a look at this bug later today. Thanks for the update.
Flags: needinfo?(hskupin)
Comment 82•6 years ago
|
||
I forgot to mention that I submitted a try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3cbeebb14f223fc46b8007d7cb215371715c027 Please check for the results later.
Comment 83•6 years ago
|
||
mozreview-review |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review253014 ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:59 (Diff revision 4) > + self.assertIsInstance(rect["y"], int) > + self.assertIsInstance(rect["height"], int) > + self.assertIsInstance(rect["width"], int) > > def test_set_types(self): > - for x, y in (["a", "b"], [1.2, 3.4], [True, False], [[], []], [{}, {}]): > + illegal_rects = (["a", "b", "h", "w"], Mind changing the variable name to `invalid_rects`? ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:184 (Diff revision 4) > # but we don't have this information. It should be safe to > # assume a window can be moved to (0,0) or less. > elif os == "linux": > # certain WMs prohibit windows from being moved off-screen > - self.assertLessEqual(position["x"], 0) > - self.assertLessEqual(position["y"], 0) > + self.assertGreaterEqual(new_position["x"], 0) > + self.assertGreaterEqual(new_position["y"], 0) I still think this needs to be `assertLessEqual`. In your case and in automation we have a WM which might set it to 0, and as such all the tests are passing, but would fail if a WM allows -8/-8. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:191 (Diff revision 4) > # On macOS, windows can only be moved off the screen on the > # horizontal axis. The system menu bar also blocks windows from > # being moved to (0,0). > elif os == "darwin": > - self.assertEqual(-8, position["x"]) > - self.assertEqual(23, position["y"]) > + self.assertEqual(-8, new_position["x"]) > + self.assertEqual(23, new_position["y"]) Hm, interestingly this fails for me on OS X in non-headless mode with: > AssertionError: 23 != 22 But this failure has not been introduced with your patch, so it would need to be investigated in a different bug. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:206 (Diff revision 4) > self.max["width"] * 2, self.max["height"] * 2) > - new = self.marionette.window_size > + actual_size = self.marionette.window_rect > > # in X the window size may be greater than the bounds of the screen > - self.assertGreaterEqual(new["width"], self.max["width"]) > - self.assertGreaterEqual(new["height"], self.max["height"]) > + self.assertGreaterEqual(self.max["width"], new_size["width"]) > + self.assertGreaterEqual(self.max["height"], new_size["height"]) Please revert the order of the arguments in both methods. Your changes shouldn't have modified any of the existent expectations. ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:216 (Diff revision 4) > - result = self.marionette.set_window_rect(width=self.max['width'], > + expected_size = self.marionette.set_window_rect(width=self.max["width"], > - height=self.max["height"]) > + height=self.max["height"]) > - self.assertEqual(result["width"], self.max["width"]) > - self.assertEqual(result["height"], self.max["height"]) > + result_size = self.marionette.window_rect > + > + self.assertGreaterEqual(self.max["width"], expected_size["width"]) > + self.assertGreaterEqual(self.max["height"], expected_size["height"]) We resize to the available screensize, and as such it should not be greater. Please revert the changes here. Also as best do not close all the issues. At least not those were a discussion is pending.
Attachment #8974311 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 84•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review253014 > Mind changing the variable name to `invalid_rects`? Changed. > I still think this needs to be `assertLessEqual`. In your case and in automation we have a WM which might set it to 0, and as such all the tests are passing, but would fail if a WM allows -8/-8. Reverted. > We resize to the available screensize, and as such it should not be greater. Please revert the changes here. > > Also as best do not close all the issues. At least not those were a discussion is pending. Reverted.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 86•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review253014 > Please revert the order of the arguments in both methods. Your changes shouldn't have modified any of the existent expectations. I still get failure on both height and width.
Comment 87•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review253014 > I still get failure on both height and width. Mind pasting the exact failure with the new code?
Comment 88•6 years ago
|
||
mozreview-review |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review253518 While a new try build is running please fix the commit message. If all tests are passing we should land your patch, and move any other test failure out to a new bug. ::: commit-message-0cd10:5 (Diff revision 5) > +Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. > + > +MozReview-Commit-ID: 83Z4dpIbwFQ > +*** > +Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. Somehow those two additional lines sneaked in.
Attachment #8974311 -
Flags: review?(hskupin) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 90•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review253518 > Somehow those two additional lines sneaked in. Ah, removed and pushed.
Assignee | ||
Comment 91•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review253014 > Mind pasting the exact failure with the new code? FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen - AssertionError: 1356 not greater than or equal to 1366 FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen - AssertionError: 758 not greater than or equal to 768
Comment 92•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review253014 > FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen - AssertionError: 1356 not greater than or equal to 1366 > > FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen - AssertionError: 758 not greater than or equal to 768 For the failure details it would be great to also get the stack so I can see in which line this is happening, and the trace log to see which data got transferred. But for now can I assume that your screen resolution is 1366x768?
Comment 93•6 years ago
|
||
Maybe you have already seen the results from the try build. If not here those are: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6649c42bca03b278f4ddea5cdf402acf58915941 As you can see there are failures across platforms for the test `test_resize_larger_than_screen`. Lets take the example for Linux: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6649c42bca03b278f4ddea5cdf402acf58915941&selectedJob=180697744 Via an execute_script call we retrieve the screen max values: https://treeherder.mozilla.org/logviewer.html#?job_id=180697744&repo=try&lineNumber=25470-25471 Those return 1600x1200. Then we set a start size of 1280x1040. As next step the 2xmax size should be set for both width and height which should be 3200x2400, but please check what is done: https://treeherder.mozilla.org/logviewer.html#?job_id=180697744&repo=try&lineNumber=25474 > [0,8,"WebDriver:SetWindowRect",{"y":2400,"x":3200,"width":null,"height":null}] We do not send a command to adjust the size but to move the window to a new position. And as such the assertions afterward fail as expected. It means you have to fix the arguments for the `set_window_rect` call.
Comment 94•6 years ago
|
||
mozreview-review |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review254604 ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:201 (Diff revision 6) > - self.assertEqual(old["width"], new["width"]) > - self.assertEqual(old["height"], new["height"]) > > def test_resize_larger_than_screen(self): > - self.marionette.set_window_size( > + new_size = self.marionette.set_window_rect( > self.max["width"] * 2, self.max["height"] * 2) As pointed out this call is wrong.
Attachment #8974311 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 95•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review254604 > As pointed out this call is wrong. Edited every set_window_rect call to explicitly naming arguments to match parameters. No more tests fail. I will learn to navigate the try server output better.
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Attachment #8967606 -
Flags: review?(hskupin)
Comment 97•6 years ago
|
||
(In reply to Venkatesh from comment #95) > Edited every set_window_rect call to explicitly naming arguments to match > parameters. No more tests fail. Great to hear that! I will trigger another try build to verify that. > I will learn to navigate the try server output better. Let me know if you have questions in regards of that or anything else. I'm happy to explain.
Comment 98•6 years ago
|
||
mozreview-review |
Comment on attachment 8974311 [details] Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. https://reviewboard.mozilla.org/r/242630/#review254988 It looks all great now! Thanks a lot for all your work on this bug. We can finally land your changes.
Attachment #8974311 -
Flags: review?(hskupin) → review+
Comment 99•6 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/70c13cd61037 Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. r=whimboo
Comment 100•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/70c13cd61037
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•6 years ago
|
Comment 101•6 years ago
|
||
Sorry for the wrong information on IRC regarding the uplift request. We indeed need it on beta and for esr60. For now lets uplift to beta first. Thanks.
status-firefox60:
wontfix → ---
status-firefox-esr60:
--- → affected
Whiteboard: [lang=py][lang=js] → [lang=py][lang=js][checkin-needed-beta]
Comment 102•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/e4e135dd13f5
Whiteboard: [lang=py][lang=js][checkin-needed-beta] → [lang=py][lang=js]
Comment 103•6 years ago
|
||
Beta looks fine so please also uplift the test-only patch to esr60.
Whiteboard: [lang=py][lang=js] → [lang=py][lang=js][checkin-needed-esr60]
Comment 104•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/af365070668f
Whiteboard: [lang=py][lang=js][checkin-needed-esr60] → [lang=py][lang=js]
Comment 105•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/ef91a3faffbf
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•