search

TestRoots / watchdog

IntelliJ & Eclipse plugin for monitoring how Java applications are developed and tested
http://www.testroots.org
Other
18 stars 10 forks source link

Initial implementation of static analysis listeners #279

Closed TimvdLippe closed 6 years ago

TimvdLippe commented 6 years ago

Built on top of #278, interesting commit is https://github.com/TestRoots/watchdog/commit/6e723a2d5c453b768bb317c9933a7208130e0a6a.

This PR introduces an initial implementation of the static analysis listener. The hardest and most confusing part of the listener is to start listening at the correct time. In IntelliJ, there is no way to know which warnings were generated based on the "initial pass" of the code analyzer when you open your editor. Since these warnings are already tracked, we want to ignore these, but do listen to subsequent changes.

The logic to make sure we only listen for the second batch and further is as follows:

  1. First we have to wait till the editor finished indexing. This makes sure that the initial code analyzer "success" event is dodged.
  2. Then once we receive the global code analyzer event, we need to check whether this event is actually for the file that we were tracking. To do so, we need to check the FileDirtyStatus of the document we are watching.

If both events happened, then we start listening for warnings that are added and removed from the model. If they are a WARNING, the trackingManager receives the corresponding event. This is UNKNOWN for now, as I have not anonymized the warnings yet.

Note that this sends ALL warnings to the server. We will probably want to do some simple clustering (for a timeline) on the client first, to not overload the server with events. I did introduced a concept of delayed processing (warning.creationTime.plus(CREATION_TRIGGERED_DELAY).isBeforeNow()), to cluster at that point in time.

We might want to empirically determine the value of CREATION_TRIGGERED_DELAY to see how granular we want to make our data. This is maybe something to discuss with @azaidman ?

All in all, I would want to hold off publishing to WatchDog, before we implement the clustering logic, to prevent overloading the server at this point.

Fixes #280

TimvdLippe commented 6 years ago

@Inventitech All right, this should be the initial implementation of the listeners in both Eclipse and IntelliJ. In Intellij we had to do some dirty work to get the correct event. In Eclipse, the diffing algorithm was necessary as Eclipse trashes and recreates all markers.

Inventitech commented 6 years ago

We are going to need a couple of tests to verify the base functionality of this. :)

TimvdLippe commented 6 years ago

@Inventitech Current status of this PR: It took a lot of debugging, but I finally got the Eclipse UI tests passing on Travis. This means that the test story for Eclipse is solid now. I still need to update the IntelliJ implementation to only work on save.

Inventitech commented 6 years ago

I know this is WIP (and great W so far!), but could you the tests seem to have grown pretty lengthy and complicated to understand. Could you describe them in method comments? Thanks!

TimvdLippe commented 6 years ago

@Inventitech In general I prefer lengthy tests that clearly describe (in code) each test, rather than shortened tests. To me, the tests are very clear. Could you therefore add some comments on the test lines that you are having trouble understanding, then I can add comments there? :smile:

TimvdLippe commented 6 years ago

@Inventitech Implementation is mostly finished now. I am going to try to write some IntelliJ tests to make sure that is working as well. I think it would be best if we try this out locally first before publishing a new version.

Inventitech commented 6 years ago

Current tests contain at least a class documentation. Especially for tests longer than 10 lines, I want a brief summary to understand what it does. Most tests are not difficult, but the amount of code takes a lot of time to read. Sometimes, I am only interested in what scenarios have been covered. I can't get this info right now without spending a good 10 minutes.

TimvdLippe commented 6 years ago

@Inventitech Most of the feedback has been processed.

TimvdLippe commented 6 years ago

@Inventitech Friendly ping for next week :smile:

TimvdLippe commented 6 years ago

Note that this effort was split into two. This PR contains the initial implementation. The code is later revised and extended in #282 which includes the warning classification. The build fully works if you first merge this branch and then #282.

TimvdLippe commented 6 years ago

@Inventitech In the last commit I have fixed the updatesite. Apparently the Eclipse plugin has been published ever since, but users never got an update as the updatesite url was wrong (it was not pointing to an actual p2 repository).

To resolve this issue, I propose to turn http://updatesite.testroots.org/ into an p2 repository itself (by simply copying from /updatesite/ to /). This should start notifying existing users of the actual update. Then with this fix, they will actually be redirected to the correct location and then should continue receiving updates.