Closed Bug 1432105 Opened 6 years ago Closed 6 years ago

Stop pointer action tests from restarting Firefox

Categories

(Testing :: geckodriver, enhancement, P1)

Product:
Testing
Component:
geckodriver
Version:
Version 3
Type:
enhancement

Tracking

(firefox60 fixed, firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: whimboo, Assigned: impossibus)

References

Details

Attachments

(3 files)

Bug 1432105 - Stop pointer action tests from restarting Firefox;
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1432105 - Disable test_pause_dblclick;
59 bytes, text/x-review-board-request
whimboo
: review+
Details
pointer-beta.patch
18.26 KB, patch
Details | Diff | Splinter Review 
Once bug 1421878 is fixed we should no longer have to use `new_session` for pointer action tests, which would safe us a ton of time because right now Firefox gets restarted in between most every test.
Assignee: nobody → mjzffr
Depends on: 1432773
Blocks: 1434618
Comment on attachment 8950237 [details]
Bug 1432105 - Stop pointer action tests from restarting Firefox;

https://reviewboard.mozilla.org/r/219492/#review225236

This looks fine. Maybe once it landed on try, trigger a couple more wd jobs to verify we are stable. Thanks!
Attachment #8950237 - Flags: review?(hskupin) → review+
Priority: -- → P1
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 87942fdd64759946003d6b55f64dfc2cc8edaad9 -d 850a68535455: rebasing 446932:87942fdd6475 "Bug 1432105 - Stop pointer action tests from restarting Firefox; r=whimboo" (tip)
merging testing/web-platform/meta/MANIFEST.json
warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9878ed59f9a6
Stop pointer action tests from restarting Firefox; r=whimboo
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/9497 for changes under testing/web-platform/tests
It looks like that with this patch Maja hit our mystic problem with pytest and that every time! Maybe this gives us the opportunity to finally get this investigated and fixed! James, can you have a look?
Flags: needinfo?(james)
I would love to look at this, and will eventually, but I have a huge backlog of wptsync issues to look at in the next few days. Maybe ato has some time to look at this sooner? Otherwise I can look at it once the sync is in a better place.

ato: if you have time then my suggested tactic is to us gdb with the python extensions and run py-bt in each python process  to figure out where they are. Most of course will be just waiting on some lock, but the important thing is what the executor process is doing when it's hung. Presumably pytest will be somewhere on the stack.
Flags: needinfo?(james) → needinfo?(ato)
I’m afraid I don’t either.  My top priority is still making
the window tracking patches ready for review so we have chance of
l anding them this quarter.
Flags: needinfo?(ato)
Fixing this will likely also fix Bug 1411845, although now I'm not sure it's really a pytest problem. In any case, at least it's reliably reproducible now.
Blocks: 1411845
(In reply to Henrik Skupin (:whimboo) from comment #8)
> It looks like that with this patch Maja hit our mystic problem with pytest
> and that every time! Maybe this gives us the opportunity to finally get this
> investigated and fixed! James, can you have a look?

I was actually wrong here. The problem seems to be that Firefox starts very slowly and as such the Marionette listener gets started around 25s later! So only 5s are remaining for the test, which is too less.

Maja, could you rebase the patch so that it depends on latest m-c? Would be good to get another try build running.
Depends on: 1443437
Btw the patch needs a rebase now due to conflicts in the MANIFEST. I rebased locally and pushed a try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9660bf58776d69f91e762bf7dfaa2901b138509

Maja, can you please update your patch? It would be great to get this out today.
It looks like we are still failing a lot in mouse_dblclick.py, which is actually unrelated to bug 1443437. The problem is that we hang infinitely in `performActions` for `test_no_dblclick`, and then get killed:

[task 2018-03-08T09:55:36.057Z] 09:55:36     INFO - STDOUT: tests/web-platform/tests/webdriver/tests/actions/mouse_dblclick.py::test_no_dblclick
[..]
[task 2018-03-08T09:55:36.149Z] 09:55:36     INFO - PID 856 | 1520502936134	geckodriver::marionette	TRACE	-> 346:[0,578,"performActions",{"actions":[{"actions":[{"origin":"viewport","type":"pointerMove","x":58,"y":190},{"button":0,"type":"pointerDown"},{"button":0,"type":"pointerUp"},{"duration":650,"type":"pause"},{"button":0,"type":"pointerDown"},{"button":0,"type":"pointerUp"}],"id":"pointer_id","parameters":{"pointerType":"mouse"},"type":"pointer"}]}]
[task 2018-03-08T09:56:04.557Z] 09:56:04     INFO - TEST-UNEXPECTED-TIMEOUT | /webdriver/tests/actions/mouse_dblclick.py | expected OK

Maja, will you be able to have a look at this problem or has someone else to check that?
Flags: needinfo?(mjzffr)
There are two intermittents now, based on extra logging in https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc1669b9defab85fe302b17abac9ea916eec36cb:

(1) We hang during a "pause" action (tests/web-platform/tests/webdriver/tests/actions/mouse_dblclick.py::test_no_dblclick  and actions/mouse_dblclick.py::test_dblclick_with_pause_after_second_pointerdown)

(2) We hang during a "pointermove" action (tests/web-platform/tests/webdriver/tests/actions/modifier_click.py::test_modifier_click[\ue050-shiftKey])

That's all the investigation I have time for now. I can revisit this again on Monday if no one else gets to it.
Flags: needinfo?(mjzffr)
Comment on attachment 8960848 [details]
Bug 1432105 - Disable test_pause_dblclick;

https://reviewboard.mozilla.org/r/229602/#review235368

Thank you Maja for having a look into that problem! Please see my inline comments for those things which aren't that clear yet.

::: commit-message-97c8f:1
(Diff revision 1)
> +Disable test_pause_dblclick; r?whimboo

I really wonder if we also have to skip the modifier test which hangs. I think it's the shift key one (See the try job results).

::: testing/web-platform/tests/webdriver/tests/actions/mouse_dblclick.py
(Diff revision 1)
>      assert len(events) == 8
>      filtered_events = [filter_dict(e, expected[0]) for e in events]
>      assert expected == filtered_events[1:]
> -
> -
> -def test_dblclick_with_pause_after_second_pointerdown(session, test_actions_page, mouse_chain):

Can't we just skip individual files via @skip, or why do we have to move all the to skip tests into a new module? This looks like a huge overhead.

::: testing/web-platform/tests/webdriver/tests/actions/mouse_pause_dblclick.py.ini:1
(Diff revision 1)
> +from tests.actions.support.mouse import get_inview_center, get_viewport_rect

I assume you wanted to name this file `mouse_pause_dblclick.py` and not `mouse_pause_dblclick.py.ini`?

::: testing/web-platform/tests/webdriver/tests/actions/mouse_pause_dblclick.py.ini:32
(Diff revision 1)
> +        assert len(events) == 8
> +        filtered_events = [filter_dict(e, expected[0]) for e in events]
> +        assert expected == filtered_events[1:]
> +
> +
> +def test_no_dblclick(session, test_actions_page, mouse_chain):

Why do we have to disable also this test?
Attachment #8960848 - Flags: review?(hskupin) → review-
Comment on attachment 8960848 [details]
Bug 1432105 - Disable test_pause_dblclick;

https://reviewboard.mozilla.org/r/229602/#review235368

> I really wonder if we also have to skip the modifier test which hangs. I think it's the shift key one (See the try job results).

Yeah, I was on the fence. It's a less frequent intermittent, and likely due to a different issue (not pauses), so I decided to leave it enabled.

> Can't we just skip individual files via @skip, or why do we have to move all the to skip tests into a new module? This looks like a huge overhead.

That's what I wanted to do of course, but as far as I can tell you can only disable entire filed, not individual tests functions.

> I assume you wanted to name this file `mouse_pause_dblclick.py` and not `mouse_pause_dblclick.py.ini`?

whoops!

> Why do we have to disable also this test?

That's actually the test that fails most often, based on my recent try pushes, because of the long-ish pause.
Comment on attachment 8960848 [details]
Bug 1432105 - Disable test_pause_dblclick;

https://reviewboard.mozilla.org/r/229602/#review235542

Sad to see that we do not have fine-graned control about skipping individual tests. We should really get this added. So looks fine to me then.
Attachment #8960848 - Flags: review?(hskupin) → review+
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 7 changes to 5 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev 943189a73c0c needs "Bug N" or "No bug" in the commit message.
remote: Maja Frydrychowicz <mjzffr@gmail.com>
remote: Disable test_pause_dblclick; r=whimboo
remote: 
remote: MozReview-Commit-ID: CSe3ABNS7hX
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f6b8d9c6cbfa
Stop pointer action tests from restarting Firefox; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/f424114c6a2c
Disable test_pause_dblclick; r=whimboo
https://hg.mozilla.org/mozilla-central/rev/f6b8d9c6cbfa
https://hg.mozilla.org/mozilla-central/rev/f424114c6a2c
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Upstream PR was closed without merging
Blocks: 1436995
Is this something we can backport to Beta? Bug 1436995 is still hitting there.
Flags: needinfo?(mjzffr)
Flags: needinfo?(mjzffr)

    
    

    
  
Sheriffs, please uplift attached patch to beta, test-only. Thanks.
Whiteboard: [checkin-needed-beta]

    
    

    
  
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10414 for changes under testing/web-platform/tests

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: