Skip to content

Configure stubtest hook for stub testing #415

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

Merged
merged 1 commit into from
Feb 1, 2022
Merged

Configure stubtest hook for stub testing #415

merged 1 commit into from
Feb 1, 2022

Conversation

kasium
Copy link
Contributor

@kasium kasium commented Dec 21, 2021

❓ What kind of change does this PR introduce?

  • 🐞 bug fix
  • 🐣 feature
  • πŸ“‹ docs update
  • πŸ“‹ tests/coverage improvement
  • πŸ“‹ refactoring
  • πŸ’₯ other

πŸ“‹ What is the related issue number (starting with #)

Part of #384

❓ What is the current behavior? (You can also link to an open issue here)

No stub testing.

❓ What is the new behavior (if this is a feature change)?

This pre-commit hook (which doesn't seem to exist yet, therefore we need a local one) will check if the .pyi files matching the .py files. With this, I already found several issues which are fixed in this commit as well

πŸ“‹ Contribution checklist:

(If you're a first-timer, check out
this guide on making great pull requests)

  • I wrote descriptive pull request text above
  • I think the code is well written
  • I wrote good commit messages
  • I have squashed related commits together after
    the changes have been approved
  • Unit tests for the changes exist
  • Integration tests for the changes exist (if applicable)
  • I used the same coding conventions as the rest of the project
  • The new code doesn't generate linter offenses
  • Documentation reflects the changes
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences

This change is Reviewable

@kasium kasium mentioned this pull request Dec 21, 2021
3 tasks
@codecov

This comment has been minimized.

@kasium
Copy link
Contributor Author

kasium commented Dec 26, 2021

@webknjaz why does the CI fail?

@webknjaz
Copy link
Member

@webknjaz why does the CI fail?

Because of #415 (comment) (plus the commit check fails because of the commit message format).

@kasium kasium changed the title Add stubtest hook Add stubtest hook for stub testing Jan 11, 2022
@kasium kasium changed the title Add stubtest hook for stub testing Configure stubtest hook for stub testing Jan 11, 2022
@kasium
Copy link
Contributor Author

kasium commented Jan 11, 2022

Okay, I configured now the setting and improved the commit message.
However the check is still failing but I guess this is not related to me error: pathspec 'stubtest' did not match any file(s) known to git

@webknjaz
Copy link
Member

However the check is still failing but I guess this is not related to me error: pathspec 'stubtest' did not match any file(s) known to git

Yeah, that one is broken. We need to report a bug to the action upstream. But https://github.com/cherrypy/cheroot/runs/4780587437?check_suite_focus=true is a legitimate failure that should be fixed.

@webknjaz webknjaz added the enhancement Improvement label Jan 12, 2022
@kasium
Copy link
Contributor Author

kasium commented Jan 12, 2022

However the check is still failing but I guess this is not related to me error: pathspec 'stubtest' did not match any file(s) known to git

Yeah, that one is broken. We need to report a bug to the action upstream. But https://github.com/cherrypy/cheroot/runs/4780587437?check_suite_focus=true is a legitimate failure that should be fixed.

Can you please do that? Also codeclimate complains, but the page is a 404 error...

@kasium kasium requested a review from webknjaz January 12, 2022 19:58
@webknjaz webknjaz assigned webknjaz and unassigned kasium Jan 12, 2022
@webknjaz
Copy link
Member

Can you please do that?

Sure, I can fix it. I think that it probably just requires some extra deps with types.

Also codeclimate complains, but the page is a 404 error...

Oh, that is weird. It's 404 for me too. Will have to look into it separately.

@webknjaz
Copy link
Member

@kasium I've rebased this on top of the master with fixed CI and figured out how to reuse the present mypy repo definition and call stubtest from there. But now it's returning some errors. Could you figure out how to ignore them?

@webknjaz webknjaz assigned kasium and unassigned webknjaz Jan 22, 2022
@kasium
Copy link
Contributor Author

kasium commented Jan 22, 2022

@webknjaz to be honest I don't work with pre-commit in my projects, so I can't analyse this in detail. If you run stubtest with the arguments directly it works fine...

What I figured out is, that for whatever reasons, the args are not applied. If you use e.g. this config, it works:

    entry: stubtest cheroot --mypy-config-file mypy.ini --allowlist stubtest_allowlist.txt
    args: []

However, then this error shows up:

error: cheroot failed to import: No module named 'cheroot'
Stub: at line 1
MypyFile:1(
  /root/git/cheroot/cheroot/__init__.pyi)
Runtime:
MISSING

I have no clue why this happens, but I guess this is related to mypy. stubtest works fine. Not sure, but MAYBE it's way easier to move the pyi files to typeshed and then to add inline type hints when you decide to drop python2?

@webknjaz webknjaz assigned webknjaz and unassigned kasium Jan 26, 2022
@webknjaz
Copy link
Member

webknjaz commented Jan 26, 2022

What I figured out is, that for whatever reasons, the args are not applied. If you use e.g. this config, it works:

@kasium thanks for looking into it. I just realized that args values like --mypy-config-file mypy.ini are passed as a single subprocess argument while it should be either separate ('--mypy-config-file', 'mypy.ini') (just like bash parses them) or with an equals ('--mypy-config-file=mypy.ini') so that the argument parser could figure out that these are option+value pairs.

(This is because if --mypy-config-file mypy.ini comes in as a monolithic value in sys.argv, the parser would "see that" as just a key named 'mypy-config-file mypy.ini')

I'll try to see what can be done on this level and report back.

@webknjaz
Copy link
Member

Hacking it to use PYTHONPATH=. makes the import cheroot pass but reveals other problems with missing deps. I suppose it's best to revert to using a local hook after all.

@webknjaz
Copy link
Member

Not sure, but MAYBE it's way easier to move the pyi files to typeshed

I think adding places to watch over is going to increase the synchronization+maintanance burden. So I'd rather stick with the current approach.

@webknjaz
Copy link
Member

@kasium I think I've made the pre-commit+tox setup correct now. But I'm seeing some errors that seem to be legitimate.
May I ask you to take a look at the logs?
https://github.com/cherrypy/cheroot/runs/4956669107?check_suite_focus=true#step:12:40

@webknjaz webknjaz assigned kasium and unassigned webknjaz Jan 26, 2022
@kasium
Copy link
Contributor Author

kasium commented Jan 27, 2022

@webknjaz I fixed the issues (see also comments in the ignore file)


ssl_conn_type: SSL.Connection
ssl_conn_type: Type[SSL.Connection]
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if this could be made optional (because it is in runtime).

This hook is not declared officially so this addition piggy-backs on
what the MyPy repository provides redefining the invocations.

It checks if the ``.pyi`` files match the ``.py`` files.

Part of #384
@webknjaz
Copy link
Member

webknjaz commented Feb 1, 2022

@kasium thanks for fixing that! I've also fixed the dependencies (because the current pyi definition assumes that pyOpenSSL is always present in the env while in fact, it's optional in runtime):

MyPy stubtest................................................................Failed
- hook id: stubtest
- exit code: 1

error: cheroot.ssl.pyopenssl.ssl_conn_type is not present at runtime
Stub: at line 6
Type[OpenSSL.SSL.Connection]
Runtime:
MISSING

@webknjaz webknjaz merged commit b99c3da into cherrypy:master Feb 1, 2022
@kasium kasium deleted the stubtest branch February 3, 2022 16:45
@kasium
Copy link
Contributor Author

kasium commented Feb 3, 2022

Cool, thanks!

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

Successfully merging this pull request may close these issues.

2 participants