1127823 - Make use of unique parameter types in close_all() of Windows class

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is from the discussion on this PR:
https://github.com/mozilla/firefox-ui-tests/pull/50/files#r23735855

As of now the close_all() method allows window exceptions being passed in as handles or BaseWindow. Chris mentioned his concerns about this because it can be disruptive.

So we should figure out what we want to do here. I'm happy to let this library and especially this method work with BaseWindow instances. So usually all of our tests make use of those. Handles shouldn't be touched at all. And in the unit tests we can simply create a BaseWindow class from a handle to push it into this method.

Chris, would that be a proper solution for you?

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Sorry, I wasn't clear. I find APIs that take a list or a single element and convert it to a list internally really problematic. I know from past discussions that this is pretty common in Python APIs, and I'm probably in a minute minority on this point. I just mentioned it in case you were sympathetic and were OK with just always passing a list.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Well, I have no real objection here, so I'm happy to go the route what you propose. Barbara, maybe you can work on a fix today?

Comment 3

[Barbara Miller (:galgeek)]

Attachment: github_pull_request.txt

As I’ve commented on github, I am sympathetic both to Henrik's suggestion that close_all should expect a particular parameter type and to Chris’s distaste for APIs that convert a single element to a list, so I've implemented both here, in case that's what everybody wants after all.

Comment 4

[Barbara Miller (:galgeek)]

I've updated the github PR per Henrik's feedback there.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8557540 [details] [review]
github_pull_request.txt

I think this is an improvement, thanks!

Comment 6

[Barbara Miller (:galgeek)]

I've again updated the github PT per Henrik's feedback there.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

PR merged as https://github.com/mozilla/firefox-ui-tests/commit/b6c5183d1ceeaeacc315d9d56222559ed8504a3c