Add dummy as a known platform, and use it in tests

[bruno-rino]

Signed-off-by: Bruno Rino bruno.rino@gmail.com

When creating a widget in a TestCase, the factory always needs to be specified, as in:

        self.switch = toga.Switch(self.text,
                                  on_change=self.on_change,
                                  value=self.value,
                                  enabled=self.enabled,
                                  factory=toga_dummy.factory)

My suggestion is to add dummy to the list of know platforms, and manually set the platform in the TestCase subclass. This would avoid the repetitive factory=toga_dummy.factory.

I tested this against a single widget, and the test passed as intended.

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

[bruno-rino]

Notes:

1. I did not update the tests themselves; I figured I would wait and see what your response was.
2. Codecov reports include switch.py and test_switch.py, which I didn't include in the commit. That puzzles me.

[freakboy3742]

So - I get the intent here; but the use of the factory argument is at least partially intentional. Over the years, I've learned the hard way that global variables and singletons are always the the hardest things to test, and area always the things that get in the way when you least expect it. Explicit arguments and passing of context is a 100% reliable way of avoiding those problems.

As case in point - you haven't made any changes to tests, and the changes you have made looks like they should be completely passive unless you're in the dummy module... and yet Windows and GTK tests are failing. There's an import side effect that is causing those tests to start using the dummy module as the backend.

That said - I completely understand that:

1. Remembering to pass in the factory every time is inconvenient.
2. The current platform factory setup doesn't make it easy to explicitly choose a platform.

While I'm not supportive of the approach taken here to set the dummy platform as an import side effect, I would be in favour of making it easier to set the current factory as a side effect of TestCase setup (or a pytest-style fixture).

[bruno-rino]

Global variables and singletons: can't live with them, can't live without them.

Setting the current factory will always be a global operation: in the end, we're talking about overriding sys.platform, which is a global variable.

Using TestCase's setUp and tearDown to set and reset it would still break the platform backend tests, if those happen to run in parallel with the dummy backend tests.

Oh well.

[mhsmith]

Using TestCase's setUp and tearDown to set and reset it would still break the platform backend tests, if those happen to run in parallel with the dummy backend tests.

I don't think running tests in parallel is something we need to support: this would break the many existing tests that override global state.

I would be in favour of making it easier to set the current factory as a side effect of TestCase setup (or a pytest-style fixture).

I'm not very familiar with pytest yet, but the cleanest way I can think of doing this is to use an "autouse" fixture. Some restructuring may be necessary to make sure the fixture covers the correct set of tests.

[bruno-rino]

Here is a solution using setUp and tearDown.

Sorry for all the commits. I'm failing to keep the CI happy. I can't figure out how to load toga_gtk

[freakboy3742]

toga_gtk can't be imported because it hasn't been installed. The CI environment is only installing toga_core and toga_dummy.

[bruno-rino]

I don't know how to make codecov happy.

[freakboy3742]

Codecov is a coverage tool. When it runs the test suite, it's verifying every line of code that is actually executed - and if a line of code you've added in a patch isn't executed by the test suite, then it's untested code.

In 2f6bca4, the blocks of code aren't ever run because you're literally telling them not to run - so, why are they in the test at all? Of course, they're in the test because that's the feature that you're adding, so they need to run.

As I noted previously, the toga_gtk backend isn't available because it isn't installed in the CI environment. You don't fix that by skipping the parts of the test that require toga_gtk - you fix it by installing toga_gtk in the CI environment. That's what the configuration files in the .github/workflows folder and tox.ini do.

Of course, it's not quite that simple, because if the test cases require toga-gtk is installed, then no developer on Windows or macOS will be able to run the test suite. So, you'll need to work out a version of the test case that verifies this behaviour in a platform independent way.

[bruno-rino]

So, you'll need to work out a version of the test case that verifies this behaviour in a platform independent way.

That's a pretty tall order, given that the tests in question are checking that the native platform still works after my changes.

The only feasible approach I can see would be to figure out a tox+github CI configuration that installs toga_gtk only for the CI run, not on developer machines. Sounds OK to you?

[bruno-rino]

... or I could use mocks.

I'm mocking toga_gtk if it fails to import.
It might be better (and self documenting) to check for github CI environment variables (see https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables), but that would make the test specific to the github CI. I don't know how much that matters.

[freakboy3742]

So - mocking is definitely one approach you could take; however, in this case it's incomplete. If you're on a Mac, and you run the test suite through tox, the tests fail - because the test environment will only install toga-core and toga-dummy, and the test is only working around the absence of toga-gtk.

Looking at the code a little closer - a simpler approach doesn't require mocking at all. The tests that are failing in CI (and in tox) are failing because get_platform_factory() is unable to resolve the "default" platform factory. For the purposes of the test, that's perfectly acceptable behavior; you just need to know that it hasn't picked up the dummy factory. On that basis, you can change the test_get_platform_factory() test to:

    def test_get_platform_factory(self):
        try:
            factory = get_platform_factory()
            self.assertIsNotNone(factory)
            self.assertNotEqual(factory, toga_dummy.factory)
        except ModuleNotFoundError:
            # The default platform factory couldn't be imported;   
            # It can't be the dummy, and the dummy isn't ever the default platform.
            pass

Either get_platform_factory() does return a factory, and that factory isn't the dummy; or it fails to return a factory - either way, it isn't the dummy factory, which is the behavior you need to verify (although it might be nicer if the error was a Toga-specific "UnsupportedPlatformError" rather than a generic ModuleNotFoundError, so we can rule out other edge cases such as an import problem inside a backend.

In terms of this specific PR, the other change that is required is proof that this actually works in a test - you've added the plumbing to support the dummy platform being picked up without explicitly using a factory argument, but then you're not actually using it anywhere. I don't know if you were intending to attack this in a future PR, or in a future commit to this PR; but at the very least, having one example of the new behavior to prove it actually works as intended would be desirable.

That said - I'm starting to wonder if there's a bigger problem here. This entire problem exists because get_platform_factory() uses sys.platform to identify which backend should be loaded. This make overriding the default behavior difficult (as testing demonstrates); however, there are other complications. For example, a third party can't write a toga-qt backend without us adding some sort of qt-detecting behavior to get_platform_factory()

I'm wondering if we can address this testing issue in a different way - by exploiting distutils extension points (e.g., how Briefcase registers it's backends). If Toga backends registered their factories as a distutils entry point, Toga would be able to look for the backend that has registered, and just use that. In toga-core's tests, toga-dummy is the only backend that is present; on most app installs, there will be a single installed backend. This would allow users to write their own backends if they wanted, or use a backend that isn't the "toga sys.platform default". I'm not sure how distutils responds to having multiple modules register the same entry point, but that should probably be an error case regardless. I can't think of an obvious reason why you'd need multiple backends in a single virtual environment; if there is a good reason, I'm sure we can work out some sort of override behavior.

[bruno-rino]

I'm wondering if we can address this testing issue in a different way - by exploiting distutils extension points (e.g., how Briefcase registers it's backends).

Now that's interesting. I have no experience with "distutils extension points", but I'll look into it.

Apparently, importlib is recommended instead (see alert box in https://setuptools.pypa.io/en/latest/pkg_resources.html).

[mhsmith]

For discovering the entry_points at runtime, importlib.metadata is now recommended instead of pkg_resources. But both APIs use the same data, which ultimately comes from setup.cfg.

[freakboy3742]

@mhsmith That's a good point; I've modified the implementation use by Briefcase in beeware/briefcase#840.

[bruno-rino]

In my dev environment, I actually have both toga_core and toga_dummy.

That's because I installed my dev environment following these instructions:
https://toga.readthedocs.io/en/latest/how-to/contribute.html

Where, at some point, it reads:

(venv) $ pip install -e src/core
(venv) $ pip install -e src/dummy
(venv) $ pip install -e src/cocoa

toga_dummy is needed in order to run tests and coverage according to the instructions.
I presume you have moved on to using only tox, and that the contribute page needs to be updated?

Using entrypoints (and having only one backend installed) will force the use of tox for running the tests. I ran tox for the first time today!
Things I am going to miss:

1. coverage reports with lines not covered. pytest --cov --cov-report term-missing in tox.ini fixes that, but maybe you have another way?
2. run a single test while I'm developing (e.g. python -m unittest tests.test_platform)

---

The entrypoints would be something like this (keys taken from briefcase's setup.cfg):

[options.entry_points]
toga.platforms =
    # android = toga_android
    # django = toga_web
    # iOS = toga_iOS
    # linux = toga_gtk
    macOS = toga_cocoa
    # tvOS = toga_tvOS
    # watchOS = toga_watchOS
    # wearos = toga_wearOS
    # windows = toga_winforms

Maybe toga.backends is a better group name?

---

FYI: There is no uniqueness in this metadata. If two packages are defined with:

[options.entry_points]
toga.platforms =
    macOS = toga_cocoa

and

[options.entry_points]
toga.platforms =
    macOS = toga_swiftui

You'll end up with

[options.entry_points]
toga.platforms =
    macOS = toga_cocoa
    macOS = toga_swiftui

[freakboy3742]

In my dev environment, I actually have both toga_core and toga_dummy.

That's why I said there was likely a need to work out some sort of disambiguation behavior.

toga_dummy is needed in order to run tests and coverage according to the instructions. I presume you have moved on to using only tox, and that the contribute page needs to be updated?

The current development instructions work with the current development code. The development instructions may need to be updated if we change how we register backends.

Using entrypoints (and having only one backend installed) will force the use of tox for running the tests.

Ideally, it wouldn't force the use of tox, but if that turns out to be a requirement, I'm not going to lose a whole lot of sleep over it.

I ran tox for the first time today! Things I am going to miss:

1. coverage reports with lines not covered. pytest --cov --cov-report term-missing in tox.ini fixes that, but maybe you have another way?
2. run a single test while I'm developing (e.g. python -m unittest tests.test_platform)

If you modify L20 of the tox.ini to read pytest --cov --cov-report term-missing {posargs}, it will both (a) include the missing lines by default, and (b) pass on any command line arguments from tox following a --; so, for example, tox -e py310 -- tests/widgets would run just the widget tests. I'd be happy to merge a PR with that change.

FWIW, there are some other tox-config-related fixes that are required; for example, we're not currently using tox for the tests in the individual platform backends, and we probably should be.

[bruno-rino]

This is the "merged" list of entry points:

[options.entry_points]
toga.backends =
    android = toga_android
    iOS = toga_iOS
    linux = toga_gtk | toga_qt
    macOS = toga_cocoa | toga_qt
    tvOS = toga_tvOS
    watchOS = toga_watchOS
    wearOS = toga_wearOS
    windows = toga_winforms | toga_uwp | toga_qt
    dummy = toga_dummy
    terminal = toga_curses
    web = toga_django | toga_falcon | toga_pyramid | toga_starlette | toga_tornado | toga_web

The keys were taken from briefcase, but:

• I made up some keys, mainly for nursery backends ("dummy", "terminal" and "web")
• In briefcase entrypoints, there's a (commented out) "django" key, I'm using "web" instead
• "toga_qt" got 3 platform keys!
• Note that there is no "wearOS" ("wearos" in briefcase) in the nursery, or pypi

I also removed most references to toga_dummy.factory from the tests. Some were kept, for bind() functions

[bruno-rino]

I guess I need to up my environment variables game. Here I was constantly using export, when there are tools to automate this.

I found out that in bash/zsh I can set an environment variable just for the current command. I'm happy, as a developer, doing this:

TOGA_PLATFORM=dummy python -m unittest

And I think it's the easiest suggestion to add in the docs.

---

I'm concerned about the winforms backend. It keeps timing out, and I don't know if that's my fault.
I even submitted a separate, incomplete PR (about loading images from bytes) mostly to see if the winforms issue manifested there too.

[freakboy3742]

I'm concerned about the winforms backend. It keeps timing out, and I don't know if that's my fault. I even submitted a separate, incomplete PR (about loading images from bytes) mostly to see if the winforms issue manifested there too.

The problem was platform selection - the tests were running on winforms, but the test suite was trying to run the core test suite, so the core tests were running with the winforms backend. One of the tests called info_dialog(), and was waiting for user input.

I've modified the CI configuration to be explicit about the platform selection, and turned up verbosity when running the tests so it's a little more obvious in the logs when a single test is hanging.

[freakboy3742]

The implementation of TOGA_PLATFORM that you've provided addresses the immediate issue - being able to run the core test suite when you've got a platform-specific backend installed - but I'm not sure it's the right approach for the bigger problem.

What you've effectively provided is a way to override sys.platform so you can set the platform to 'dummy'. However, it won't let me resolve the case where I have 2 candidate backends for the same platform (e.g., a Qt backend and a GTK backend installed on Linux; or a cocoa and swiftui backend on macOS). I'd suggest a more direct TOGA_BACKEND=toga_dummy will be more useful in the long run.

Development documentation also needs to be updated; it still advises that you can install dummy and cocoa/gtk/winforms and run pytest.

[bruno-rino]

I've changed the implementation to use TOGA_BACKEND=toga_dummy and updated the documentation.

There's a couple of changes I am not sure of:

• I haven't changed the override_current_platform function to override the backend instead of the platform. At this point, I'm quite happy with the environment variable solution. I don't see the need for this function at all. What do you think? Remove?
• Some of the text in this section is still relevant, but most is not. Should I go ahead and edit it?
• I presume the windows terminal doesn't allow setting an environment variable just for the following command, and that the set commands work as advertised.

[codecov]

Codecov Report

Merging #1534 (9491665) into main (ba970f3) will increase coverage by 1.09%.
The diff coverage is 96.80%.

Impacted Files	Coverage Δ
src/core/src/toga/widgets/base.py	88.33% <60.00%> (-1.42%)	⬇️
src/core/src/toga/platform.py	90.47% <89.74%> (+73.80%)	⬆️
src/core/src/toga/app.py	82.85% <100.00%> (+0.50%)	⬆️
src/core/src/toga/command.py	97.05% <100.00%> (+0.08%)	⬆️
src/core/src/toga/fonts.py	85.29% <100.00%> (+1.96%)	⬆️
src/core/src/toga/icons.py	86.36% <100.00%> (+1.74%)	⬆️
src/core/src/toga/images.py	96.87% <100.00%> (+7.98%)	⬆️
src/core/src/toga/widgets/activityindicator.py	100.00% <100.00%> (ø)
src/core/src/toga/widgets/box.py	100.00% <100.00%> (ø)
src/core/src/toga/widgets/button.py	100.00% <100.00%> (ø)
... and 23 more

[freakboy3742]

I've made a couple of small tweaks (including addressing the points you've raised in your most recent comment); I think this is now ready to go. Thanks for the excellent work on this, and apologies for the delay in getting it merged!