Diagnostics for opened files #186

tomv564 · 2017-04-20T20:52:54Z

This is a small implementation towards #162:

ProjectManager exposes files to compiler via ProjectConfiguration in didOpen/didChange
ProjectManager passes a DiagnosticsHandler to ProjectConfiguration
~~Expose programSyncEvents on ProjectConfiguration, triggered after compilation~~
~~DiagnosticsPublisher requests diagnostics on a synced program and publishes per file~~
ProjectConfiguration passes TS file diagnostics to DiagnosticsHandler
~~Basic tests~~
- Unit tests for DiagnosticsPublisher
- Existing functional tests expanded to test correct uri and content update handling

Uncertainties:

It feels totally reasonable to expose a file to the compiler on didOpen, but for some reason this was not being done yet?

As discussed, VS Code stops showing diagnostics on broken code when you close the file. We would have to remove the file from the compiler to achieve consistent behaviour with VS Code.

Missing test scenarios for broken tsconfig, missing node modules, etc.

A minimum delay of 1 second feels good on instantly-compiling projects, but perhaps it should be the editor's job to delay showing feedback while the user is typing? Or should the didChange handler not trigger compiles immediately?

Thanks for the answers on these! Continuing conversation below.

codecov · 2017-04-20T21:07:13Z

Codecov Report

Merging #186 into master will increase coverage by 0.89%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
+ Coverage   54.65%   55.54%   +0.89%     
==========================================
  Files          15       16       +1     
  Lines        1945     1991      +46     
  Branches      311      319       +8     
==========================================
+ Hits         1063     1106      +43     
+ Misses        764      763       -1     
- Partials      118      122       +4

Impacted Files	Coverage Δ
src/lang-handler.ts	`11.9% <0%> (+11.9%)`	⬆️
src/typescript-service.ts	`76.57% <100%> (+0.13%)`	⬆️
src/project-manager.ts	`79.2% <78.57%> (+0.02%)`	⬆️
src/diagnostics.ts	`84.84% <84.84%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86f7223...b243451. Read the comment docs.

felixfbecker · 2017-04-21T12:07:21Z

Thanks for the PR! It is quite hard for me to understand what is going on here (some documentaiton on DiagnosticsPublisher would be helpful). What I got:

ProjectConfiguration now emits an event with a new project whenever it is updated
The ProjectManager subscribes to this event, but it does so by being passed to ProjectManager, which passes it to ProjectConfiguration, which then passes an event Subject to a method.
The DiagnosticsPublisher takes a Subscribable to subscribe on and maintains a map of file to collected diagnostics. On a program update, it updates the map, then publishes them all.

Problems I have:

The point of evented API is that the event producer is not coupled to (doesn't know about) the event consumer. The producer just emits the event, the consumer subscribes to it.
The subscribe method looks like the the one defined in the Rx Subscribable interface, but does the exact opposite. It doesn't take a Subscriber, it takes a Subscribable to subscribe on.
Subjects are an anti pattern. If you cannot avoid them, their scope should be limited - publicly exposed should be only the Observable interface.
I think using an Observable/Subject for the program update event is overkill. Are there any benefits over making the ProjectConfiguration and ProjectManager a simple EventEmitter?
The unit test for ProjectManager should just test the unit under test (ProjectManager). That means it should just assert that the program update event is emitted.

It feels totally reasonable to expose a file to the compiler on didOpen, but for some reason this was not being done yet?

No reason, makes sense to me. I think it didn't matter because we read all files in anyway and all operations ensured the files they needed.

As discussed, VS Code stops showing diagnostics on broken code when you close the file. We would have to remove the file from the compiler to achieve consistent behaviour with VS Code.

I wouldn't be too worried about that, I don't have a problem with showing diagnostics for closed files.

A minimum delay of 1 second feels good on instantly-compiling projects, but perhaps it should be the editor's job to delay showing feedback while the user is typing? Or should the didChange handler not trigger compiles immediately?

I would expect the client to not spam didChange on every key stroke, so I don't see that much of an issue with publishing diagnostics immediately.

felixfbecker · 2017-04-24T07:49:11Z

Please ping when this is ready for another review

tomv564 · 2017-04-24T21:10:51Z

I've addressed points from the initial review, thanks for taking the time!

It is quite hard for me to understand what is going on here

Added docstrings, comments and broke up DiagnosticsPublisher's update function

Using Rx is overkill

Agreed, ProjectConfiguration now just calls updateFileDiagnostics on DiagnosticsPublisher

The unit test for ProjectManager should just test the unit under test (ProjectManager)

Created unit tests to test the details of DiagnosticsPublisher's work
Kept the functional tests (titled 'Diagnostics') to catch regressions in the system

I have some time to work on this, but feel free to work on my branch!

tomv564 · 2017-04-25T16:42:54Z

Ping @felixfbecker, ready for another review!

felixfbecker · 2017-04-25T16:52:22Z

src/diagnostics.ts

+		const diagnosticsByFile = this.groupByFile(diagnostics);
+
+		// add empty diagnostics for fixed files, so client marks them as resolved
+		this.problemFiles.forEach(file => {


Use for of loops

felixfbecker · 2017-04-25T16:54:55Z

src/diagnostics.ts

+	 * @param diagnostics All diagnostics received in an update
+	 */
+	private groupByFile(diagnostics: ts.Diagnostic[]): Map<string, ts.Diagnostic[]> {
+		const diagnosticsByFile: Map<string, ts.Diagnostic[]> = new Map();


Prefer const diagnosticsByFile = new Map<string, ts.Diagnostic[]>();

felixfbecker · 2017-04-25T16:55:46Z

src/project-manager.ts

-		config.syncProgram(span);
+		config.ensureConfigFile();
+
+		// If file was unknown, we add it (which syncs program)


How can a didChange ever happen without a didOpen?

felixfbecker · 2017-04-25T16:58:04Z

src/project-manager.ts

+		if (!program) {
+			return false;
+		}
+		let changed = false;


this variable is unneeded

felixfbecker · 2017-04-25T17:02:46Z

src/project-manager.ts

+	 * Ensures a single file is available to the LanguageServiceHost
+	 * @param filePath
+	 */
+	ensureSourceFile(filePath: string): boolean {


As I understand it, the return type indicates whether the program was resynced as part of the method call? Why exactly is that information needed for a caller who wants to ensure a source file? If it is needed to decide whether to republish diagnostics, then this puts unnecessary burden on the callers to always check the return value and act on it - emitting an event is better in that case

felixfbecker · 2017-04-25T17:06:34Z

src/test/diagnostics.test.ts

+
+	beforeEach(() => {
+		publishSpy = sinon.spy();
+		langClient = {


Since you want to stub all and only spy on one I would use sinon.createStubInstance(RemoteLanguageClient)

felixfbecker · 2017-04-25T17:25:28Z

src/test/diagnostics.test.ts

+});
+
+// Integration tests that include TS compilation.
+describe('Diagnostics', () => {


Still not convinced we should have these here.

The expected behaviour of the ProjectManager is to call the DiagnosticHandler, so there should be tests in the ProjectManager suite that assert it is called. That's where all the setup/teardown code is located.

All tests are to prevent regressions

Right you are, these have been replaced with some unit tests on didChange/didOpen.

tomv564 · 2017-04-25T21:22:00Z

@felixfbecker thanks for the guidance, hope the didChange and test changes are what you intended!

felixfbecker · 2017-04-28T09:20:54Z

src/project-manager.ts

 		config.getHost().incProjectVersion();
-		config.syncProgram(span);
+		config.syncProgram();


why the removal of the span?

felixfbecker · 2017-04-28T09:21:23Z

src/diagnostics.ts

+		this.problemFiles.clear();
+
+		// for each file: publish and set as problem file
+		diagnosticsByFile.forEach((diagnostics, file) => {


felixfbecker · 2017-04-28T09:22:06Z

src/test/diagnostics.test.ts

+
+describe('DiagnosticsPublisher', () => {
+	let langClient: LanguageClient;
+	let diagnosticsManager;


felixfbecker · 2017-04-28T09:22:30Z

src/test/diagnostics.test.ts

+	let langClient: LanguageClient;
+	let diagnosticsManager;
+	const createTSFileDiagnostic = (message: string, file: ts.SourceFile) => {
+		return {


use arrow function without return or named function

felixfbecker · 2017-04-28T09:25:53Z

src/test/diagnostics.test.ts

+	beforeEach(() => {
+		publishSpy = sinon.spy();
+		langClient = sinon.createStubInstance(RemoteLanguageClient);
+		langClient.textDocumentPublishDiagnostics = publishSpy;


all methods are stubs now, so no need to assign the spy here. Just type the langClient as something like

{ [K in keyof LanguageClient]: LanguageClient[K] & sinon.SinonStub } // or LanguageClient & { textDocumentPublishDiagnostics: Function & sinon.SinonSpy }

felixfbecker · 2017-04-28T09:27:33Z

src/test/project-manager.test.ts

+		it('should compile opened file and return diagnostics', async () => {
+			projectManager.didOpen('file:///src/dummy.ts', 'const num: number = "banana";');
+			const lastArgs = diagnosticsSpy.lastCall.args[0];
+			assert.equal(lastArgs.length, 1);


Pretty sure there is a sinon assert function for this. Should also first assert that the function is called at all?

Added an assert as suggested, but I couldn't find a way to assert on the last call only with sinon's asserts. They suggest using sinon.assert.calledWith, and we would need a sinon.match to avoid check all of TS's diagnostics type. I tried this briefly but ran out of time, let me know if you want me to switch it to this syntax.

felixfbecker · 2017-04-28T09:28:39Z

src/typescript-service.ts

@@ -67,7 +68,6 @@ export type TypeScriptServiceFactory = (client: LanguageClient, options?: TypeSc
 * underscore.
 */
 export class TypeScriptService {
-


felixfbecker · 2017-04-28T09:29:59Z

src/diagnostics.ts

+	 * Requires a connection to the remote client to send diagnostics to
+	 * @param remoteClient
+	 */
+	constructor(private remoteClient: LanguageClient) {}


rename to client

felixfbecker · 2017-04-28T09:30:38Z

src/diagnostics.ts

+	 * The files that were last reported to have errors
+	 * If they don't appear in the next update, we must publish empty diagnostics for them.
+	 */
+	private problemFiles: Set<string> = new Set();


prefer type inference

felixfbecker · 2017-04-28T09:31:45Z

src/diagnostics.ts

+		const diagnosticsByFile = new Map<string, ts.Diagnostic[]>();
+		for (const diagnostic of diagnostics) {
+			// TODO for some reason non-file diagnostics end up in here (where file is undefined)
+			const fileName = diagnostic.file ? diagnostic.file.fileName : '';


const fileName = diagnostic.file && diagnostic.file.fileName

tomv564 · 2017-04-28T11:45:14Z

Thanks for the review, apologies for the unintended changes!

felixfbecker · 2017-04-29T18:03:02Z

Thanks! Great work

felixfbecker added the feature label Apr 21, 2017

tomv564 force-pushed the file-diagnostics branch from 680099b to 807ec7b Compare April 23, 2017 19:33

tomv564 force-pushed the file-diagnostics branch from 469e0ef to d13ed60 Compare April 24, 2017 20:52

Use single publisher, add tests

bb0210a

tomv564 force-pushed the file-diagnostics branch from d13ed60 to bb0210a Compare April 24, 2017 20:53

Merge branch 'master' into file-diagnostics

bd396eb

felixfbecker reviewed Apr 25, 2017

View reviewed changes

tomv564 added 5 commits April 25, 2017 21:51

Use for-of loops for array interation

b00243e

Use typed Map() constructor

3736e18

Simplify ensureSourceFile and didChange

7c5c714

Use sinon.createStubInstance, remove integration tests

f13c046

Add didOpen and didChange tests to ProjectManager

dee0233

tomv564 added 2 commits April 27, 2017 23:33

Merge branch 'master' into file-diagnostics

223cf94

filter compiler diagnostics out if they happen to be added

a6a8599

felixfbecker suggested changes Apr 28, 2017

View reviewed changes

tomv564 added 3 commits April 28, 2017 12:55

Merge branch 'master' into file-diagnostics

36b4c04

Review fixes (part 1)

a8acd04

Improve use of spies in tests

9f2c9b9

felixfbecker added 3 commits April 29, 2017 19:38

Merge branch 'master' into file-diagnostics

08d15a6

Add return type

c7a1efa

Minor improvements

b243451

felixfbecker approved these changes Apr 29, 2017

View reviewed changes

felixfbecker merged commit da4451a into sourcegraph:master Apr 29, 2017

tomv564 deleted the file-diagnostics branch September 9, 2017 09:40

jzashch mentioned this pull request Aug 9, 2018

Send diagnostics for all open files #499

Open

Diagnostics for opened files #186

Diagnostics for opened files #186

Uh oh!

Conversation

tomv564 commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

felixfbecker commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixfbecker commented Apr 24, 2017

Uh oh!

tomv564 commented Apr 24, 2017

Uh oh!

tomv564 commented Apr 25, 2017

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!

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!

tomv564 commented Apr 25, 2017

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!

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!

tomv564 commented Apr 28, 2017

Uh oh!

felixfbecker commented Apr 29, 2017

Uh oh!

Uh oh!

tomv564 commented Apr 20, 2017 •

edited

Loading

codecov bot commented Apr 20, 2017 •

edited

Loading

felixfbecker commented Apr 21, 2017 •

edited

Loading