PR: Fix introspection testing (CI)

[dalthviz]

Description of Changes

Properly handle instrospection usage via the Completions plugin configuration

Issue(s) Resolved

Fixes #22012

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: dalthviz

[jitseniesen]

@dalthviz Can you please give a bit of explanation about this PR so that I can review it? What went wrong and how does this fix it?

[dalthviz]

Sure @jitseniesen ! So basically here I:

• Removed the ad hoc way to disable/enable instrospection when testing that was using an env variable (SPY_TEST_USE_INTROSPECTION) and instead changed things to rely on each completion provider enabled config value (for example CONF.set('completions', ('enabled_providers', 'lsp'), False) to disable the languageserver provider and CONF.set('completions', ('enabled_providers', 'lsp'), True) to enable it)
  • Doing the above, I noticed an issue related with the logic to close/call the shutdown logic of the completion providers which prevented closing the completion plugin when no provider (lsp, fallback and snippets) is enabled (the can_close |= True changes).
• Added a missing wait call for the fallback provider actor following the logic used over the other completion providers.

Related to how these changes fix things, my guess is that the initial migration of the Editor caused the previous logic in place for handling the introspection for the tests (via an env var) to not be enough. This due to the Editor initialization logic now taking place alongside the other plugins which, regardless of the env var, makes the Editor interact with the completion plugin. I would say the main problem is that the env var approach doesn't deal with the providers still appearing from the config side of things as enabled 🤔

My local testing suggested that the changes here prevent seeing the QThread warning and the test process to crash when exiting but let me know if they work in your end too!

[jitseniesen]

I tested it locally and it does get rid of the QThread error. It is also much neater without the environment variable.

I have one substantive (and rather long, sorry!) comment on the can_close function, mostly about the existing code which you probably did not write. Let me know what you think.

I also made two comments (about CONF and running_under_pytest) about points that are probably fine, so just saying it's fine is enough to satisfy me.

[jitseniesen]

Just checking ... it is not necessary to undo these changes in the CONF after the test, is it?

[dalthviz]

I don't think so but to be sure checked and seems like the logic that enables not worrying about possible effects to the config after running a test is at:

spyder/spyder/config/base.py

Lines 219 to 224 in 071e646

	def get_conf_path(filename=None):
	"""Return absolute path to the config file with the specified filename."""
	# Define conf_dir
	if running_under_pytest() or get_safe_mode():
	# Use clean config dir if running tests or the user requests it.
	conf_dir = get_clean_conf_dir()

So when running tests, a clean dir is used for the config 👍

[jitseniesen]

Why did you remove the running_under_pytest() condition?

[dalthviz]

Oh, I overlooked the comment 😅 Will readd the running_under_pytest() validation and the comment 👍

[jitseniesen]

I think this is just a complicated way to write can_close = True (assuming that can_close is always a bool).

Actually, I don't understand the logic of the whole can_close function, regardless of your change. It seems to me that the function should return False if there is at least one provider that is running for which provider.can_close() returns False. This is also what the docstring indicates. So if there are two running providers, one of which can close and the other can not, then the function should return False. However, I believe that's not what the code does. In fact, it returns True if there is at least one provider for which provider.can_close() returns True. So in the case that there are two running providers, one of which can close and the other can not, then the function actually return True.

Additionally, I don't like the use of the bitwise | operator instead of the logical or operator.

For clarity, this is how I would write the function (but I'm not sure that is the intended meaning):

    def can_close(self) -> bool:
        """Check if any provider has any pending task."""
        can_close = True
        for provider_name in self.providers:
            provider_info = self.providers[provider_name]
            if provider_info['status'] == self.RUNNING:
                provider = provider_info['instance']
                provider_can_close = provider.can_close()
                can_close = can_close and provider_can_close
        return can_close

Or maybe:

    def can_close(self) -> bool:
        """Check if any provider has any pending task."""
        for provider_name in self.providers:
            provider_info = self.providers[provider_name]
            if provider_info['status'] == self.RUNNING:
                provider = provider_info['instance']
                if not provider.can_close():
                    return False
        return True

What do you say, does this make sense?

[dalthviz]

You are totally right! So indeed this logic should be preventing the plugin close if any of the running providers is unable to be closed. Also, I think this logic is following a pattern like the one used over the plugins registry to check if things can be close/deleted:

spyder/spyder/api/plugin_registration/registry.py

Lines 573 to 599 in 071e646

	def can_delete_all_plugins(self,
	excluding: Optional[Set[str]] = None) -> bool:
	"""
	Determine if all plugins can be deleted except the ones to exclude.
	Parameters
	----------
	excluding: Optional[Set[str]]
	A set that lists plugins (by name) that will not be deleted.
	Returns
	-------
	bool
	True if all plugins can be closed. False otherwise.
	"""
	excluding = excluding or set({})
	can_close = True
	# Check external plugins
	for plugin_name in (
	set(self.external_plugins) | set(self.internal_plugins)):
	if plugin_name not in excluding:
	can_close &= self.can_delete_plugin(plugin_name)
	if not can_close:
	break
	return can_close

So my guess is that the original idea of the check was to do something like your first suggestion but using &= So something like:

    def can_close(self) -> bool:
        """Check if any provider has any pending task."""
        can_close = True
        for provider_name in self.providers:
            provider_info = self.providers[provider_name]
            if provider_info['status'] == self.RUNNING:
                provider = provider_info['instance']
                can_close &= provider.can_close()
        return can_close

We could even do:

    def can_close(self) -> bool:
        """Check if any provider has any pending task."""
        can_close = True
        for provider_name in self.providers:
            provider_info = self.providers[provider_name]
            if provider_info['status'] == self.RUNNING:
                provider = provider_info['instance']
                can_close &= provider.can_close()
                if not can_close:
                     break
        return can_close

Maybe your second suggestion (with the early return) is better? 🤔 What do you think @ccordoba12 ?

[ccordoba12]

Maybe your second suggestion (with the early return) is better? 🤔 What do you think @ccordoba12 ?

I like that suggestion. I think it's simpler and easier to follow.

[jitseniesen]

Same remark as for the function can_close() above.

[dalthviz]

I think the same applies for this, so indeed, the method should return True only if all the providers can be closed and return False otherwise 👍

[ccordoba12]

One small question for you @dalthviz, the rest looks good to me.

[ccordoba12]

Looks good to me, thanks @dalthviz!

[jitseniesen]

All fine now, thanks Daniel!