/vscode-python forked from DonJayamanne/pythonVSCode

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

Multiple linter support failing #2571

Closed
DonJayamanne opened this Issue Sep 13, 2018 · 9 comments

Comments

Assignees

@ericsnowcurrently ericsnowcurrently

@d3r3kk d3r3kk

Labels
P0 external contributor feature-linting type-bug unplanned
Projects
None yet
Milestone
  2019, week 1 - Ja...
6 participants
@DonJayamanne @d3r3kk @brettcannon @nullie @ericsnowcurrently @pvscbot
@DonJayamanne
Copy link

DonJayamanne commented Sep 13, 2018

Here's the output from the logs:

2018-09-12T23:52:33.0234844Z     ✖ Multiple linters -- error: No diagnostic messages.
2018-09-12T23:52:33.0235203Z 
2018-09-12T23:52:33.0236658Z ##[error]out\test\linters\lint.test.js(,): error : FAILED Linting - General Tests :: Multiple linters
2018-09-12T23:52:33.0237549Z ##[debug]Processed: ##vso[task.logissue type=error;sourcepath=D:\a\1\s\out\test\linters\lint.test.js;]FAILED Linting - General Tests :: Multiple linters
2018-09-12T23:52:33.0237762Z

@DonJayamanne DonJayamanne assigned d3r3kk Sep 13, 2018

@DonJayamanne DonJayamanne added type-bug needs PR labels Sep 13, 2018

@d3r3kk d3r3kk referenced this issue Sep 18, 2018

Merged

Skip Multiple Linters system test due to unreliability (Symptom: executeCommand's await resolves prematurely). #2609

6 of 6 tasks complete
@d3r3kk

This comment has been minimized.

Sign in to view
Copy link

d3r3kk commented Sep 18, 2018

I've skipped this test for now, as there needs to be some real work done to sort out the root cause.

The branch linting_test_failure in my personal repo here contains some console.log messages that show the behaviour symptomatic of the failure (you will likely have to run it on Windows, under Python3.7, and mutliple times before you see the failure occur).

Essentially the trouble is that the Promise on the executeCommand('python.runLinter') is resolving before the command is actually complete, and it also seems to be returning more than a single time, judging from the console output I've seen.

@brettcannon brettcannon added the feature-* label Sep 18, 2018

@DonJayamanne DonJayamanne added the P1 label Dec 13, 2018

@brettcannon

This comment has been minimized.

Sign in to view
Copy link
Member

brettcannon commented Dec 13, 2018

Attached is my log. If I have just flake8 on it's fine, and if I just have pep8 turned on it's, fine. But as soon as I have both on I get a message saying pep8 isn't installed and an error about TypeError: Converting circular structure to JSON (log).

@brettcannon brettcannon added feature-linting and removed feature-* labels Dec 13, 2018

@brettcannon brettcannon changed the title Azure CI Test failure for Multiple Linters Multiple linter support failing Dec 13, 2018

@brettcannon brettcannon unassigned d3r3kk Dec 13, 2018

@brettcannon brettcannon referenced this issue Dec 13, 2018

Closed

Check if running two linters simultaneously still works #3618

@nullie

This comment has been minimized.

Sign in to view
Copy link

nullie commented Dec 14, 2018 •
edited

https://github.com/Microsoft/vscode-python/blob/master/src/client/common/process/proc.ts#L172 method calls JSON.parse(JSON.stringify(options) (trying to make a deep copy?) , but fails on options.token._emitter._listeners

nullie added a commit to nullie/vscode-python that referenced this issue Dec 14, 2018

@nullie
Fix multiple linter support (Microsoft#2571)
51aaa75

@nullie nullie referenced this issue Dec 14, 2018

Merged

Fix multiple linter support (#2571) #3702

4 of 6 tasks complete

@d3r3kk d3r3kk added this to the 2018, week 50 - Dec sprint 2 milestone Dec 14, 2018

@d3r3kk d3r3kk added unplanned P0 and removed P1 labels Dec 14, 2018

@d3r3kk

This comment has been minimized.

Sign in to view
Copy link

d3r3kk commented Dec 14, 2018

We took a fix (#3702) that corrects the behaviour in the runtime, but now we need to actually fix the failure occurring in the tests. This is p0 for this release.

@d3r3kk d3r3kk self-assigned this Dec 14, 2018

d3r3kk added a commit that referenced this issue Dec 14, 2018

@nullie @d3r3kk
Fix multiple linter support (#2571) (#3702)
Loading status checks…
b0a54b5

d3r3kk added a commit that referenced this issue Dec 14, 2018

@nullie @d3r3kk
Fix multiple linter support (#2571) (#3702)
c16a769

d3r3kk added a commit that referenced this issue Dec 14, 2018

@d3r3kk
skipping unreliable linter test 
- tracked by #2571
- correct news entry to address the appropriate issue
Loading status checks…
898cc23

@nullie nullie referenced this issue Dec 15, 2018

Closed

getDefaultOptions refactoring #3712

6 of 6 tasks complete

@brettcannon brettcannon added the external contributor label Dec 17, 2018

@nullie

This comment has been minimized.

Sign in to view
Copy link

nullie commented Dec 19, 2018

Is the tests issue reproducible locally?
@brettcannon

This comment has been minimized.

Sign in to view
Copy link
Member

brettcannon commented Dec 19, 2018

@nullie yes

@d3r3kk d3r3kk referenced this issue Dec 19, 2018

Merged

Re-enable tests for multiple linters #3756

6 of 6 tasks complete

d3r3kk added a commit that referenced this issue Dec 20, 2018

@d3r3kk
Re-enable tests for multiple linters (#3756) 
Failing test is no longer able to repro locally.

For #2571
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
a139248

@DonJayamanne DonJayamanne assigned DonJayamanne and d3r3kk and unassigned d3r3kk and DonJayamanne Dec 31, 2018

d3r3kk added a commit to d3r3kk/vscode-python that referenced this issue Dec 31, 2018

@d3r3kk
Skip failing test for multiple linters 
- notorious for time-outs
- we will mock linting output up instead
- Microsoft#2571
fdeb0ef

d3r3kk added a commit to d3r3kk/vscode-python that referenced this issue Jan 1, 2019

@d3r3kk
Skip failing test for multiple linters 
- notorious for time-outs
- we will mock linting output up instead
- Microsoft#2571
7229c69

@DonJayamanne DonJayamanne modified the milestones: 2018, week 50 - Dec sprint 2, 2019, week 1 - Jan Sprint 1 Jan 2, 2019

d3r3kk added a commit to d3r3kk/vscode-python that referenced this issue Jan 4, 2019

@d3r3kk
Move 'Multiple linters' system test into its own file. 
Fix for Microsoft#2571

- Multiple linters test just uses the app, no mocking necessary.
  - Thus it did not truly belong in linter.test.ts
- Only needed mocked linter output stub to be a good system test
5cb6970

d3r3kk added a commit to d3r3kk/vscode-python that referenced this issue Jan 4, 2019

@d3r3kk
Move 'Multiple linters' system test into its own file. 
Fix for Microsoft#2571

- Multiple linters test just uses the app, no mocking necessary.
  - Thus it did not truly belong in linter.test.ts
- Only needed mocked linter output stub to be a good system test
658282d

@d3r3kk d3r3kk referenced this issue Jan 4, 2019

Merged

Move 'Multiple linters' system test into its own file #3866

6 of 6 tasks complete

d3r3kk added a commit that referenced this issue Jan 4, 2019

@d3r3kk
Move 'Multiple linters' system test into its own file (#3866) 
* Move 'Multiple linters' system test into its own file.

Fix for #2571

- Multiple linters test just uses the app, no mocking necessary.
  - Thus it did not truly belong in linter.test.ts
- Only needed mocked linter output stub to be a good system test
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
2e5e9b5

@ericsnowcurrently ericsnowcurrently self-assigned this Jan 7, 2019

@ericsnowcurrently

This comment has been minimized.

Sign in to view
Copy link
Member

ericsnowcurrently commented Jan 7, 2019

I'm going to validate this.
@ericsnowcurrently

This comment has been minimized.

Sign in to view
Copy link
Member

ericsnowcurrently commented Jan 7, 2019

@d3r3kk what are the steps to reproduce this? I'm not seeing the problem under the 2018.12.1 release. Is this WIndows-only?
@ericsnowcurrently

This comment has been minimized.

Sign in to view
Copy link
Member

ericsnowcurrently commented Jan 7, 2019

tests are passing now

@ericsnowcurrently ericsnowcurrently closed this Jan 7, 2019

@pvscbot pvscbot removed the needs PR label Jan 7, 2019

@lock lock bot locked as resolved and limited conversation to collaborators Feb 4, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.