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

ExtensionPoint: return True on successful validate() #503

Merged
merged 2 commits into from
May 3, 2021

Conversation

minrk
Copy link
Contributor

@minrk minrk commented May 3, 2021

Related to #500, but doesn't actually fix the main issue there (failure to identify extensions that are not importable).

Current situation:

  • ExtensionPoint.validate always returns a falsy value (None or False)
  • ExtensionPackage only checks the falsiness, which means it always returns False
  • ListServerExtensionApp never checked validate return value, meaning all extensions were always considered valid, even though all extensions actually self-reported as invalid

This fixes:

  • ExtensionPoint.validate returns True on successful validation
  • ListServerExtensionApp checks the return value of validate

The original error causing failed validation is still swallowed and not logged, which should probably be addressed at some point as well, but at least now values are correct.

This came up when I was trying to write some tests validating an image, where I had:

def test_extensions():
    from jupyter_core.paths import jupyter_config_path
    from jupyter_server.extension.manager import ExtensionManager
    from jupyter_server.extension.config import ExtensionConfigManager

    m = ExtensionManager(
        config_manager=ExtensionConfigManager(read_config_paths=jupyter_config_path())
    )
    for name, ext in m.extensions.items():
        print(f"Validating extension {name}")
        assert ext.validate()

and was surprised to see that all the extensions were actually invalid.

minrk added 2 commits May 3, 2021 11:14
and include validate() in test coverage, which fails prior to the change because validate() returned None
ExtensionPoint.validate returns False when any error is raised

ListServerExtension does not check the return value, only errors
@codecov-commenter
Copy link

codecov-commenter commented May 3, 2021

Codecov Report

Merging #503 (0a86f7c) into master (5e77b73) will increase coverage by 0.05%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #503      +/-   ##
==========================================
+ Coverage   77.68%   77.73%   +0.05%     
==========================================
  Files         106      106              
  Lines        9276     9280       +4     
  Branches     1001     1002       +1     
==========================================
+ Hits         7206     7214       +8     
+ Misses       1711     1706       -5     
- Partials      359      360       +1     
Impacted Files Coverage Δ
jupyter_server/extension/serverextension.py 75.35% <0.00%> (-0.54%) ⬇️
jupyter_server/extension/manager.py 89.56% <100.00%> (+2.82%) ⬆️
jupyter_server/tests/extension/test_manager.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e77b73...0a86f7c. Read the comment docs.

@blink1073 blink1073 added this to the 1.7 milestone May 3, 2021
Copy link
Member

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

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

Thanks, @minrk. LGTM.

@Zsailer Zsailer merged commit ccb4e2d into jupyter-server:master May 3, 2021
hMED22 pushed a commit to hMED22/jupyter_server that referenced this pull request Jan 23, 2023
ExtensionPoint: return True on successful validate()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants