Fix bunch of issues with reference related tests by MikhailArkhipov · Pull Request #176 · microsoft/python-language-server

MikhailArkhipov · 2018-10-01T17:14:33Z

Properly resolve function arguments vs global variables
Treat reassignments as reference, not as definition
Improve CancelAnalysis test (took forever with my 6500 files library)
Fix some dispose issues (queue was not always empty after dispose/cancel)
Dispose of stuff in order
Update signature test to new behavior
Update reference tests to new behavior

AlexanderSher · 2018-10-01T17:19:09Z

                        LogMessage(MessageType.Error, t.Exception.Message);
                        return;
                    }
+                    _shutdownCts.Token.ThrowIfCancellationRequested();


It's enough to call it either here or in OnDocumentChangeProcessingCompleteAsync.

AlexanderSher · 2018-10-01T17:20:44Z

                }

                if (doc is IAnalyzable analyzable && enqueueForAnalysis) {
+                    _shutdownCts.Token.ThrowIfCancellationRequested();


We can check this unconditionally.

AlexanderSher · 2018-10-01T17:21:43Z


        private async Task OnDocumentChangeProcessingCompleteAsync(IDocument doc, VersionCookie vc, bool enqueueForAnalysis, AnalysisPriority priority, IDisposable disposeWhenEnqueued) {
            try {
+                _shutdownCts.Token.ThrowIfCancellationRequested();


Then we don't need to call _disposableBag.ThrowIfDisposed(); here (and catch it at the end).

Actually OnDocumentChangeProcessingCompleteAsync is not even async anymore

AlexanderSher · 2018-10-01T17:31:04Z

-                    foreach (var defScope in scope.EnumerateTowardsGlobal
-                        .Where(s => s.ContainsVariable(name.Name) && (s == scope || s.VisibleToChildren || IsFirstLineOfFunction(scope, s, location)))) {
-                        var scopeVariables = GetVariablesInScope(name, defScope).Distinct();
+                    foreach (var s in scope.EnumerateTowardsGlobal) {


Can we extract the body of if (expr is NameExpression name) { into separate method (something like GetVariablesFromNameExpression)?

AlexanderSher · 2018-10-01T22:38:43Z

Do you want the last change to be part of the same review?

MikhailArkhipov · 2018-10-01T22:42:59Z

Yes. It actually fixes FindReferences test and makes it work as PyCharm does. Also adds test for class local variables.

AlexanderSher · 2018-10-01T22:44:24Z

Ok, I'll take a look tomorrow morning.

AlexanderSher · 2018-10-03T14:53:47Z

 ";
                await server.SendDidOpenTextDocument(uri, text);

-                var references = await server.SendFindReferences(uri, 3, 15);
-                references.Should().OnlyHaveReferences(
+                var expected = new (Uri documentUri, (int, int, int, int), ReferenceKind?) [] {


You don't need documentUri here: var expected = new (Uri, (int, int, int, int), ReferenceKind?) [] {

Or maybe we should just add an overload for OnlyHaveReferences with (Uri documentUri, (int, int, int, int), ReferenceKind) tuple.

AlexanderSher · 2018-10-03T14:59:01Z

@@ -5341,7 +5365,7 @@ class test():

            var files = Directory.GetFiles(Path.Combine(configuration.PrefixPath, "Lib"), "*.py", SearchOption.AllDirectories);
            Trace.TraceInformation($"Files count: {files}");
-            var contentTasks = files.Select(f => File.ReadAllTextAsync(f)).ToArray();
+            var contentTasks = files.Take(Math.Min(50, files.Length)).Select(f => File.ReadAllTextAsync(f)).ToArray();


files.Take(50) is enough.
From sources:

private static IEnumerable<TSource> TakeIterator<TSource>(IEnumerable<TSource> source, int count) { if (count > 0) { foreach (TSource source1 in source) { yield return source1; if (--count == 0) break; } } }

AlexanderSher · 2018-10-03T14:59:32Z

                    await server.SendDidOpenTextDocument(new Uri(files[i]), contentTasks[i].Result);
                }

                server.AnalysisQueue.Count.Should().NotBe(0);
            } finally {
                if (server != null) {
                    analysisCompleteTask = EventTaskSources.AnalysisQueue.AnalysisComplete.Create(server.AnalysisQueue, new CancellationTokenSource(10000).Token);
-                    serverDisposeTask = Task.WhenAny(Task.Run(() => server.Dispose()), Task.Delay(1000));
+                    serverDisposeTask = Task.WhenAny(Task.Run(() => server.Dispose()), Task.Delay(5000));


Do we really need 5 seconds to dispose?

Not really.

AlexanderSher · 2018-10-03T15:02:00Z

-class MyBaseClass1(object):
-    def base_method(self): pass
+        /*
+                [TestMethod, Priority(0)]


The whole block of commented tests is offset, please undo it.

OK. Prob formatter.

Fix bunch of issues with reference related tests

MikhailArkhipov added 11 commits September 27, 2018 11:28

Merge branch 'master' of https://github.com/Microsoft/python-language…

5a5d521

…-server

Merge branch 'master' of https://github.com/Microsoft/python-language…

17b9830

…-server

Speed up CancelAnalysis

b1e5f31

Reference filtering (partial)

08d4995

Scopes

71e70f4

Ref fixes

1a78270

Function scope check

f1254b0

Merge master

d713701

Use index

e6164dc

Unused code

23973d9

Undo some

a96aa99

AlexanderSher reviewed Oct 1, 2018

View reviewed changes

Comment thread src/Analysis/Engine/Impl/ModuleAnalysis.cs

AlexanderSher reviewed Oct 1, 2018

View reviewed changes

Comment thread src/Analysis/Engine/Impl/ModuleAnalysis.cs

AlexanderSher reviewed Oct 1, 2018

View reviewed changes

Comment thread src/LanguageServer/Impl/Implementation/Server.cs

AlexanderSher reviewed Oct 1, 2018

View reviewed changes

MikhailArkhipov added 3 commits October 1, 2018 10:33

PR feedback

56d8e79

Update baseline

3679941

Split method

104bb43

AlexanderSher approved these changes Oct 1, 2018

View reviewed changes

Better scoping checks

2ee4d06

AlexanderSher reviewed Oct 3, 2018

View reviewed changes

Comment thread src/Analysis/Engine/Test/AnalysisTest.cs

AlexanderSher reviewed Oct 3, 2018

View reviewed changes

PR feedback

e744ffe

MikhailArkhipov merged commit 6a20692 into microsoft:master Oct 3, 2018

MikhailArkhipov mentioned this pull request Oct 3, 2018

FunctionScoping test fails #155

Closed

MikhailArkhipov deleted the refs2 branch October 3, 2018 17:08

MikhailArkhipov mentioned this pull request Oct 4, 2018

FindReferences test fails #156

Closed

jakebailey mentioned this pull request Oct 9, 2018

Fix function override completion test #219

Merged

jakebailey pushed a commit to jakebailey/python-language-server that referenced this pull request Nov 1, 2019

Merge pull request microsoft#176 from MikhailArkhipov/refs2

60fa595

Fix bunch of issues with reference related tests

Conversation

MikhailArkhipov commented Oct 1, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexanderSher commented Oct 1, 2018

Uh oh!

MikhailArkhipov commented Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexanderSher commented Oct 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MikhailArkhipov commented Oct 1, 2018 •

edited

Loading