Move test-specific functions to tests

[cbrnr]

This keeps the code base clean and also makes the class shorter.

[cbrnr]

I didn't know that MNE-Python tests actually require these _fake_click etc. methods. I still think that these methods should be test-only, so should we adapt the MNE tests? Can we add these methods from within MNE-Python (just like I inject them here for the tests)? Are there any other options?

[larsoner]

so should we adapt the MNE tests? Can we add these methods from within MNE-Python (just like I inject them here for the tests)? Are there any other options?

I think it's important and useful to continue testing on older versions of MNE-Python, so I don't think we should just update MNE-Python main and only test that here.

I can't think of any easy solutions offhand

[cbrnr]

I would update MNE-Python tests and just keep the methods around until the oldest supported MNE-Python does not require them anymore.

[larsoner]

I would update MNE-Python tests and just keep the methods around until the oldest supported MNE-Python does not require them anymore.

Is the idea to copy the _fake_click that currently lives here to live in MNE-Python instead?

How about a mne_qt_browser._testing.* just for the utility functions? This is similar to how there is numpy.testing and pandas.testing etc. So we move all of the tests themselves, but not the utilities that allow testing in MNE-Python. It seems cleaner to me. That way the utility functions needed for testing mne-qt-browser (e.g., over in MNE-Python) live in and are maintained by this package.

[cbrnr]

Is the idea to copy the _fake_click that currently lives here to live in MNE-Python instead?

Not necessarily. I'd prefer if they still lived in this package, just not in the main MNEQtBrowser class. Ideally, these methods should only be available to the tests. I've already done that for the tests in this package, but MNE-Python tests also need them.

[larsoner]

Okay with me to have them live in tests/utils.py and use try/except at the MNE-Python end to prefer that pattern

[cbrnr]

Okay with me to have them live in tests/utils.py and use try/except at the MNE-Python end to prefer that pattern

Yes, this is basically what I already did here. I moved all functions except _get_n_figs() and _close_all() to tests/utils.py - I think it would be nice to also move these two, because they are also only needed for tests, I think this should be possible.

Can you elaborate how you would like to handle this at the MNE-Python end? Since MNEQtBrowser inherits from BrowserBase, which defines all of these test methods as abstract methods, we are currently forced to provide concrete implementations.

One way around this would be to declare non-abstract (i.e., normal) methods in BrowserBase with stubs that raise NotImplementedError. But then we still have to handle these errors somehow. Maybe just remove them?

[larsoner]

Oh sorry I hadn't looked that closely, didn't realize they were part of the abstract base class. I'd leave them in that case. Seems like more work than it's worth to move them around / out of the class. And it is kind of nice having both backends implement the same method, even if it is only used in tests. IIRC that was part of design decisions made by @drammock so he should probably weigh in too.

[cbrnr]

I would strongly argue for separating the actual functionality from test functionality.

[drammock]

What is the end goal here? IIUC, the proposal is to trade complexity in the class definition (having some extra private methods defined that aren't needed by users, only devs) for complexity somewhere else (mutating the class, by injecting those methods, prior to testing). I don't like that idea because it means that the class being tested is not the same class being used by users. Sure, I can look at the details of this PR and convince myself that it's fine right now, but it's a bad pattern to follow IMO. I also worry that it's extra cognitive overhead for new maintainers, who may look at a test and wonder "where is this _fake_press method coming from? I don't see it defined in the class... [ctrl+f 'def _fake_press']... ok, it's injected before the test... but why is it done this way??"

(all of this is just in reference to this repo; the additional added complexity at the MNE-Python end seems like a pretty strong signal that this is not an improvement overall).

If you're really really bothered by the test-only methods being defined alongside the other methods in one place, my advice would be: make sure all the test-only methods come at the end, and put a prominent code comment in the file to delineate the transition. If that's not good enough, another possibility (but seems like overkill to me) is to define a mixin class that has the testing methods in a different file, and have the main class inherit from it. Will either of those solutions work for you?

[cbrnr]

I think it's a bad pattern to mix in test methods. This is not the purpose of that class. I don't like injecting either, I just did that because it was the least invasive solution. We could also subclass it and add the test methods that way.

[drammock]

My general feeling aligns with @larsoner:

Seems like more work than it's worth to move them around / out of the class.

However: if we can get it to work cleanly, I am OK with eliminating those methods in the main class definition. I think maybe something like this might work:

1. define a function to inject the test-only methods (done)
2. make sure that function is API-available so MNE-Python tests can use it
3. add a fixture to conftest.py that monkey-patches the class to inject the test-only methods
4. mark that fixture as auto-use so we don't have to write add_test_browser_methods in every single test where it's needed