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

Add more capability unit tests and enable coverage reporting #380

Merged
merged 6 commits into from
Oct 2, 2023

Conversation

alcarney
Copy link
Collaborator

@alcarney alcarney commented Sep 26, 2023

Description (e.g. "Related to ...", etc.)

This PR attempts to address issues around computing server capabilities raised in #377

  • With these changes I'm now able to run the jedi-language-server test suite on my machine
  • Thanks to the type annotation suggestions from @dimbleby, the _provider_options method is now annotated, allowing mypy to check it's usage.
  • The default argument to _provide_options has been made mandatory.

Also

  • I've also made an attempt at enabling test coverage - though I am not 100% sure if reporting of subprocess is working or not! 😅
  • poe will now always run all lints, even when some of them fail so that it's possible to see all failures at once.

Code review checklist (for code reviewer to complete)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated, as appropriate
  • Docstrings have been included and/or updated, as appropriate
  • Standalone docs have been updated accordingly

@dimbleby
Copy link
Contributor

(I don't care either way about the import changes: except that that putting such things into the same commit as the actual substantial change that you are making is unhelpful for reviewers!)

I'd suggest that the default value for _provider_options() should be None rather than True: and that callers who want a True default should have to say so. It sort of doesn't matter, you will feel free to ignore me, clearly it's possible to write correct code either way. But I reckon that this is what allowed the confusion that caused the bug to be written in the first place: None is the conventional default on lots of APIs and it's natural for developers to expectt the same here.

@alcarney
Copy link
Collaborator Author

(I don't care either way about the import changes: except that that putting such things into the same commit as the actual substantial change that you are making is unhelpful for reviewers!)

Apologies! I probably should've marked this as draft before, as I wasn't necessarily expecting a review 😅

@alcarney alcarney requested a review from tombh September 27, 2023 23:40
@tombh
Copy link
Collaborator

tombh commented Sep 28, 2023

I approved the PR, but @dimbleby, please chime in as well if anything stands out.

Coverage is a nice touch! Thoughts for another time:
Should we set it up so that a PR is refused if coverage goes down? We could add https://coveralls.io integration for nice graphs and stuff?

@alcarney
Copy link
Collaborator Author

Should we set it up so that a PR is refused if coverage goes down? We could add https://coveralls.io/ integration for nice graphs and stuff?

Up to you.

Another option is to copy the attrs project and have the report show up in the workflow summary. It looks like it would be fairly easy to setup

This makes the `default` argument to the `_provider_options` argument
mandatory so that each usage is explicit about the type of `value` the
following code is expecting to handle.
This should make it easier to fix errors all in one go
@tombh tombh force-pushed the capability-tests branch from 159b6e7 to d11bb9a Compare October 2, 2023 12:45
@tombh tombh merged commit 20a42ff into openlawlibrary:main Oct 2, 2023
17 checks passed
@tombh
Copy link
Collaborator

tombh commented Oct 2, 2023

Yeah having the summary in the workflow like that is nice. I'll see if I can add that soon.

@alcarney alcarney deleted the capability-tests branch November 8, 2023 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants