All apps get a default content when no content is provided. #2844
Conversation
|
If you look at the tests that are failing, they're almost all So, in this case, the failing tests are exactly the tests that need to be updated. There's likely no need for additional tests - you just need to modify existing tests to assert the new default condition of a window. |
|
I saw that the same logic is repeated a lot of times, so I fixed a few tests to get some early feedback if you don't mind :-) |
There was a problem hiding this comment.
See inline for notes on the approach you've taken.
One other helper that I'll draw your attention to in case you're not aware of it: unittest.mock.ANY. This is a value that is "equal to everything", so if you've got a test where "there needs to be a value here, but I don't care what it is", using ANY is often a good choice. Unfortunately, ANY isn't hashable, so it can't be used as a key in a dictionary - but if you've got a situation where an argument being passed in is a randomly generated string (such as the widget ID in this case), it can be handy.
| @@ -0,0 +1 @@ | |||
| When no content is provided in the Window object, default to empty Box. | |||
There was a problem hiding this comment.
| When no content is provided in the Window object, default to empty Box. | |
| A Window is now guaranteed to have a content widget. If no content is provided at time of construction, an empty Box widget will be added as the content. |
| "child1_id": child1, | ||
| "child2_id": child2, | ||
| "child3_id": child3, | ||
| }.items() |
There was a problem hiding this comment.
While this does pass, It's not a very robust test.
dict.items() will be converting the dictionary into a list of tuples; the list comparison will be trying to evaluate that the two lists sort in lexical order. In effect, this test asserts that the app.widgets list has, at the bare minimum, a single widget that is alphabetically sorted before "widget_id". This will evaluate as true in this case... but so would any app widget list that contains at least one item ({'foo': widget} would pass this test, for example, even though it doesn't contain any of the child widgets).
If the intention is to validate that "all of these objects in app.widgets", it would be better to evaluate each membership specifically; or, alternatively, use a subset-based check (possibly factored out as a standalone assertion).
Another approach in this case would be even simpler - use a reference to the literal "extra" widget that is causing an issue for the test. The extra widget that prevents the use of a literal == here is the content widget of the test app's main window - something that isn't involved in this test, but is in the app registry. Adding app.main_window.content.id: app.main_window.content, as an additional pair in the expected output is a reasonable approach in this case - we don't care about the specific ID or widget; we just care that the app's widget registry contains a reference to all known widgets - and the app's (default) content is one of those widgets.
Fixes #2818.
I didn't modify any tests since 20 tests are failing. I can modify all the tests to pass, however this seems like a big overhaul to the test suite, so I just wanted to make sure this is what we want to do.
PR Checklist: