Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 6 years ago

As noticed on bug 1403923 none of the finalizers which usually get added to a session is getting run if the `new_session` fixture is used. It only works for test methods which use the `session` fixture.

Works:
======

def test_foobar(session):
    assert 1 == 1


Fails:
======

def test_foobar(new_session, add_browser_capabilites):
    assert 1 == 1


This can cause lots of trouble for following tests, at least until the browser gets restarted.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 1 • 6 years ago

So we do not add any finalizer inside of `new_session`:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/support/fixtures.py#176-202

Instead we call `end()` twice! The first time directly while creating the session, which basically reverts the newly created session to the global one, and then once again at the end of the test.

Maybe I miss something but I don't think that we ever should call `end()` directly here, but only through the added finalizer. Doesn't it make all of those tests currently invalid?

James, I would appreciate your feedback here. Thanks.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 6 years ago

Ok, calling `end()` directly is actually used to end a former session as started by a previous test. So it would be important to keep.

	James Graham [:jgraham]
	Comment 5 • 6 years ago
mozreview-review

Comment on attachment 8937689 [details]
Bug 1425947 - Expose session capabilities in 'new_session' fixture.

https://reviewboard.mozilla.org/r/208396/#review214162

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 6 years ago
mozreview-review

Comment on attachment 8937689 [details]
Bug 1425947 - Expose session capabilities in 'new_session' fixture.

https://reviewboard.mozilla.org/r/208396/#review214166

	James Graham [:jgraham]
	Comment 7 • 6 years ago
mozreview-review

Comment on attachment 8937688 [details]
Bug 1425947 - Run session finalizers also for new_session fixture.

https://reviewboard.mozilla.org/r/208394/#review214164

::: testing/web-platform/tests/webdriver/tests/support/fixtures.py:49
(Diff revision 1)
>  
>  
>  @ignore_exceptions
> -def _dismiss_user_prompts(session):
> -    """Dismisses any open user prompts in windows."""
> -    current_window = session.window_handle
> +def _dismiss_user_prompts():
> +    """Dismiss any open user prompts in windows."""
> +    global _current_session

`global` is for things that you write to. Since the varaible isn't being rebound it's not appropriate here.

::: testing/web-platform/tests/webdriver/tests/support/fixtures.py:187
(Diff revision 1)
>      except webdriver.error.SessionNotCreatedException:
>          if not _current_session.session_id:
>              raise
>  
> -    # finalisers are popped off a stack,
> -    # making their ordering reverse
> +    # finalisers are popped off a stack, making their ordering reverse
> +    request.addfinalizer(_switch_to_top_level_browsing_context)

This seems all wrong. These should run after each test, not after each session? Presumably the browser is shut after each session anyway, so what  does this achieve?

	Andreas Tolfsen ❲:ato❳
	Comment 8 • 6 years ago
mozreview-review

Comment on attachment 8937688 [details]
Bug 1425947 - Run session finalizers also for new_session fixture.

https://reviewboard.mozilla.org/r/208394/#review214174

I’m puzzled by the changes to make a number of these fixtures and
helpers not rely on the ‘session’ fixture but on _current_session.
This will presumably mean we can no longer guarantee a session will
be available to the next test that uses the fixture?

::: testing/web-platform/tests/webdriver/tests/support/fixtures.py:34
(Diff revision 1)
> -def _ensure_valid_window(session):
> -    """If current window is not open anymore, ensure to have a valid
> -    one selected.
> +def _ensure_valid_window():
> +    """Ensure to have a valid window selected if current one has been closed."""
> +    global _current_session
> +    if not _current_session:
> +        return

The reason this function takes the ‘session’ fixture is so that the
harness can guarantee there is always a window.  This means a new
session will be created if there isn’t one.

This changes it to fail silently if there is no session.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 9 • 6 years ago
mozreview-review-reply

Comment on attachment 8937688 [details]
Bug 1425947 - Run session finalizers also for new_session fixture.

https://reviewboard.mozilla.org/r/208394/#review214174

The next test will have a `session` fixture itself, and as such would create a new session.

> The reason this function takes the ‘session’ fixture is so that the
> harness can guarantee there is always a window.  This means a new
> session will be created if there isn’t one.
> 
> This changes it to fail silently if there is no session.

Ok, I see. I'm new to fixtures so that wasn't obviously clear to me. But then all the calls like `request.addfinalizer(lambda: _dismiss_user_prompts(_current_session))` are wrong and should not have the `_current_session` as argument because it gets generated by the session fixture, right?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 10 • 6 years ago
mozreview-review-reply

Comment on attachment 8937688 [details]
Bug 1425947 - Run session finalizers also for new_session fixture.

https://reviewboard.mozilla.org/r/208394/#review214164

> This seems all wrong. These should run after each test, not after each session? Presumably the browser is shut after each session anyway, so what  does this achieve?

As we have seen on bug 1403923 the left over alerts can cause trouble during shutdown. Maybe other kind of things which we usually clean-up with the finalizers for normal sessions could cause similiar behavior. Cleaning up everything what has been opened for the current session (whether if it has been created via `session` or `new_session`) would ensure a shutdown will be successful. Or are you saying that we should not care about the clean-up for `new_session` and live with possible timeouts during shutdown?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 11 • 6 years ago

I would like to know how the intended behavior for a temporary session should be during cleanup. Don't we want to do anything and leave various windows, alerts etc open, or clearly clean all those up before shutting down the browser?

Catching a shutdown issue with Firefox like in this case is good, but it means that unexpected behavior can appear during the wdspec tests.

	James Graham [:jgraham]
	Comment 12 • 6 years ago

I feel like anything that interferes with session shutdown is a pretty serious bug and should be prioritized as such, rather than adding workarounds in the test harness to let us get away with bad behaviour.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 13 • 6 years ago

We also discussed this topic in our chitchat and we agreed on marking this as wontfix.