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 Test Case for Proper Checker Invocation #55

Closed
sco1 opened this issue Dec 19, 2019 · 2 comments · Fixed by #60
Closed

Add Test Case for Proper Checker Invocation #55

sco1 opened this issue Dec 19, 2019 · 2 comments · Fixed by #60
Assignees
Milestone

Comments

@sco1
Copy link
Owner

sco1 commented Dec 19, 2019

As seen in #54, specific parameter(s) are required in the checker function signature in order to signal to Flake8 that the checker needs to be run on the specified input. While this pitfall was identified previously, a regression was introduced by #53 that prevented Flake8 from determining that this plugin needed to be run. This case is missed by our unit tests, as the way they are mocked assumes that Flake8 has invoked the checker & provided the information that was requested.

To guard against any future regressions, a test case should be developed that invokes Flake8 on an input & checks that the flake8-annotations checker is being run. From some initial mucking around, it seems like a simple stdin test case may be a good candidate for at least an initial approach.

Using a simple test case:

def bar(x) -> None:
    pass

Flake8 should yield a TYP001 error:

$ printf "def bar(x) -> None:\n    pass\n" | flake8 -
stdin:1:9: TYP001 Missing type annotation for function argument 'x'
@sco1 sco1 self-assigned this Dec 19, 2019
@sco1 sco1 mentioned this issue Dec 19, 2019
@sco1 sco1 added the blocked label Dec 20, 2019
@sco1
Copy link
Owner Author

sco1 commented Dec 20, 2019

Blocking this for now as there seems to be an upstream issue with Coverage 5.x causing CI to fail when the proposed test is added (reported here).

While this seems to be resolved by pinning Coverage below 5.0, since this is not a critical test let's wait for the issue to be triaged before deciding on the resolving action.

@sco1 sco1 removed the blocked label Jan 2, 2020
sco1 added a commit that referenced this issue Jan 2, 2020
@sco1
Copy link
Owner Author

sco1 commented Jan 2, 2020

A workaround has been identified in the upstream issue, allowing us to unblock & add this test into the next dev branch.

The immediate issue is apparently caused by the lack of an existing .coverage file when attempting to append coverage information. By specifying parallel runs in a .coveragerc file, the issue is resolved. It is not clear why the presence of subprocess brings out this issue, but the workaround sufficies for now.

@sco1 sco1 mentioned this issue Jan 19, 2020
@sco1 sco1 added this to the v1.2.0 milestone Jan 30, 2020
@sco1 sco1 closed this as completed in #60 Feb 10, 2020
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 a pull request may close this issue.

1 participant