Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

All apps get a default content when no content is provided. #2844

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cgkoutzigiannis
Copy link
Contributor

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:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@freakboy3742
Copy link
Member

If you look at the tests that are failing, they're almost all assert window.content is None, or assert len(window.widgets) == 0 - usually in the context of a window asserting its condition after being created, or the initial condition of a window before widgets are added.

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.

@freakboy3742 freakboy3742 mentioned this pull request Sep 16, 2024
4 tasks
@cgkoutzigiannis
Copy link
Contributor Author

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 :-)
Right now the issue that I am having is that there is no way to know the default's box widget_id, so I turn all the "exact match" assertions to "contains" assertions. This works, but it does feel a bit flaky.
I am definitely open to suggestions on this.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Textual apps require window content to run
2 participants