Added Errorprone plugin

[RutaGiedryte]

I added the Errorprone plugin and disabled the checks that broke the build. Those were:

1. DoubleBraceInitialization - forcing the usage of ImmutableHashMaps and ImmutableHashSets (in up to 10 places)
2. EqualsHashCode - forcing adding the hashCode() anywhere equals() was overridden (also up to 10 places).

I refactored the code, but three tests failed (those were from AllAssignmentsTests, specifically from mocks-stubs-fakes). Since I did not see an immediate reason for that, I rolled back and just disabled these checks. However, I can try to search for that reason and enable the two checks if they are interesting for the project!

Closes #231 (part two, Checkstyle is already merged in)

[mauricioaniche]

Great MR!

Regarding the points where it fails, I'd suggest we add SuppressWarnings in them. This way we ensure new code doesn't suffer from it. We can then create a new ticket to fix the old stuff.

Regarding the test, I wasn't expecting test execution to fail. What does it say in the error message? My only guess is that errorprone is also then running in Andy's compilation step, and so when it compiles the fake students code, it breaks there. If that's the case, we should disable errorprone in the CompilationStep of Andy. There might be a way to disable it via passing extra parameters to the compiler. Or at least making the errors all warnings!

[RutaGiedryte]

I have experimented a little and observed weird results. The DoubleBraceInitialization is fine, I refactored that code again and everything works, tests pass. EqualsHashCode, however, manages to fail the tests even when warnings in all relevant classes are suppressed (so not even refactoring, just disabling the check and suppressing classes by hands does not work). The errors occur because tests expect the grade to be 100 but it is lower. The 3 failing tests belong to editTripCapacity, gradeEmailService and makeReservation. This seems to be the exact area of code which has equals() but not hashCode(). I have no idea why enabling a check and then still suppressing it would change anything, but that seems to be the case.

[mauricioaniche]

Sanity check: were these tests passing in your laptop before you did this? I'll run the CI pipeline here, let's see what happens!

(I'm without my computer this week so I can't debug with you!)

[RutaGiedryte]

Sanity check: were these tests passing in your laptop before you did this? I'll run the CI pipeline here, let's see what happens!

(I'm without my computer this week so I can't debug with you!)

I checked before enabling the EqualsHashCode check and they did work, as well as before changing anything at all. Not checking that beforehand would indeed not be very wise on my part.

[mauricioaniche]

We've had some flakyness in windows machines before. So it was worth a shot asking :)

My best bet would be that when we compile code inside Andy, errorprone is crashing things there. If you open the fixture files that these tests use, do they offend this errorprone rule?

[RutaGiedryte]

We've had some flakyness in windows machines before. So it was worth a shot asking :)

My best bet would be that when we compile code inside Andy, errorprone is crashing things there. If you open the fixture files that these tests use, do they offend this errorprone rule?

Of course, that is a very understandable question :)) The only thing offending Errorprone seems to be the classes themselves, but not the fixtures or any test code. Very weird that the compilation is successful and Errorprone is happy, but our own Andy tests are not.

I tested one more thing - with the check disabled AND suppressing in action the tests still fail. However, with the check disabled and NOT suppressing, everything works. Apparently suppressing the check causes problems then? *Edit - I now remembered what I said about it not working with refactoring and check enabled either, so I guess this is more complicated than that.

[mauricioaniche]

This is super weird.

Can you add a break point in the test that fails and inspect the Result object? In particular, whether it contains some compilation error message internally?

[mauricioaniche]

Another attempt could be to add some flags to disable errorprone in CompilationStep:57

[RutaGiedryte]

Another attempt could be to add some flags to disable errorprone in CompilationStep:57

I will try both of these tomorrow! Hopefully, something will become clear.

[RutaGiedryte]

This is super weird.

Can you add a break point in the test that fails and inspect the Result object? In particular, whether it contains some compilation error message internally?

*Edit - see below, the solution is found :))

So, I have examined this, these are the news: 1) I checked different classes in different scenarios and the Result object did not contain internal errors. 2) What reduces the grade is the failing meta tests. All other code checks are okay in all cases. 3) Not all classes break after adding/suppressing the hash code, there are only 4: Reservation class in SoftWhere (editTripCapacity), Reservation class in SoftWhere (makeReservation), GradeEmail class and EmailAddress class in gradeEmailService (any of them break the gradeEmailService test). 4) Other classes are fine even after adding the hashCode, such as SQL testing with recipes or InvoiceFilter. 5) For editTripCapacity or makeReservation, 2 out of 4 tests break (these classes have analogous tests). For gradeEmailService, 7 out of 8 tests break. The meta tests that break do line replacement, and the tests with string replacement still pass. So I guess this is the most likely cause.

[RutaGiedryte]

This is super weird. Can you add a break point in the test that fails and inspect the Result object? In particular, whether it contains some compilation error message internally?

So, I have examined this, these are the news:

1. I checked different classes in different scenarios and the Result object did not contain internal errors.
2. What reduces the grade is the failing meta tests. All other code checks are okay in all cases.
3. Not all classes break after adding/suppressing the hash code, there are only 4: Reservation class in SoftWhere (editTripCapacity), Reservation class in SoftWhere (makeReservation), GradeEmail class and EmailAddress class in gradeEmailService (any of them break the gradeEmailService test).
4. Other classes are fine even after adding the hashCode, such as SQL testing with recipes or InvoiceFilter.
5. For editTripCapacity or makeReservation, 2 out of 4 tests break (these classes have analogous tests). For gradeEmailService, 7 out of 8 tests break. The meta tests that break do line replacement, and the tests with string replacement still pass. So I guess this is the most likely cause.

Okay, very exciting news, the solution has been FOUND! (credits to my friend Aldas) The reason line replacement fails is because after adding additional lines of code one is also supposed to correspondingly adjust from where to where the code gets replaced. After adjusting line numbers everything works! A very satisfying conclusion in my eyes. I will change the line numbers and push the code today :))

[mauricioaniche]

This is great news!!! Well done!!!

So this means we had to change the exercise code so that they also comply with errorprone?

Another option would be to enable errorprone only in the Andy project and not on the assignments sub projects!

But I'd say whatever is the easiest now!

[RutaGiedryte]

This is great news!!! Well done!!!

So this means we had to change the exercise code so that they also comply with errorprone?

Another option would be to enable errorprone only in the Andy project and not on the assignments sub projects!

But I'd say whatever is the easiest now!

I needed to change some line numbers in the meta tests so that they still replace the correct lines (as adding hash codes increases the number of lines in the class) and to add the hash codes. So yes, the exercise code did change as it now contains more hash codes. I pushed the code so you can see how it looks now.

[mauricioaniche]

@RutaGiedryte did you see my remark about the implementation of the hash code ?

[RutaGiedryte]

@RutaGiedryte did you see my remark about the implementation of the hash code ?

I missed that, I'm sorry! Where can I find it?

[mauricioaniche]

I left a remark directly in the code. You should be able to see it here in the front page of the PR, or if you go to the diff page!

[RutaGiedryte]

I left a remark directly in the code. You should be able to see it here in the front page of the PR, or if you go to the diff page!

Hmm, I can't seem to see it in either of those places

[Arraying]

I can't see any inline comments in the thread or diff view either, I'm on desktop.

[mauricioaniche]

Oh it was in draft state here in the GitHub app! Submitted!