Add @NullUnmarked on constructors #133
Conversation
| System.out.println( | ||
| remainingErrors.size() + " errors remaining. Applying suppression annotations."); |
There was a problem hiding this comment.
Is this print supposed to be part of this PR?
There was a problem hiding this comment.
@msridhar Yes, I was planning to add all changes to error suppression in this PR, but it is already divided into two PRs. I can add this in a follow up PR.
|
@msridhar @lazaroclapp I noticed a problem in a followup PR for this PR and updated this one. Would you please take a look at the changes. The plan for remaining error suppression is as below:
Prior to the changes I made recently, we were adding |
There was a problem hiding this comment.
LGTM though I agree with @lazaroclapp that we should have a test here
|
@msridhar @lazaroclapp Please find the update test case here 5f25f1b. This unit test with this change would have been failed without changes in this PR. |
| new OnMethod("Main.java", "test.Main", "Main(java.lang.Object,java.lang.Object)"), | ||
| "org.jspecify.nullness.NullUnmarked")); | ||
| Assert.assertEquals( | ||
| expectedAnnotations, coreTestHelper.getConfig().log.getInjectedAnnotations()); |
There was a problem hiding this comment.
Ok, I think this test logic is correct. But also, why are we piggy-backing testing a new feature/behavior onto an existing test, rather than creating one specifically for this change?
There was a problem hiding this comment.
@lazaroclapp Yes, recently I have been incorporating some new tests cases in the existing ones. The reason behind that is that the current CI is rather a heavy process due to the unit tests architecture. Each unit test is converted to a standalone gradle project and that makes each unit test somewhat heavy. We currently have 53 unit tests and even though the procedure is parallelized ( to the number of cores), in my laptop with 8 cores it takes more than 10 minutes. We are planning to have a CI that runs all tests with all optimization disabled and that should take even more. I think it worths keeping CI light and for new features that we would like to test if:
- The code pattern is very small (in this case just a constructor)
- The code pattern is not so different from other existing tests.
- The selected unit tests to be embedded is testing the similar behavior with new feature.
- Does not add too much noise to the selected test case.
We may be able to avoid adding a new test case and just include it in existing ones. Please let me know your thoughts on this and if this is a good approach. Thank you very much.
There was a problem hiding this comment.
Ok, I understand the infrastructure limitations now. I still think generally speaking such "combined" unit tests are a bad practice: a unit test, properly speaking, should be the smallest possible piece of code required to test a specific functionality in isolation. Given arbitrary engineering resources, I think it would be desirable to change the testing infra so that test cases are either much faster or this "combination" happens transparently from the point of view of a user running tests.
That said, that's obviously a massive undertaking (not to mention one which introduces its own complexities and potential errors while debugging tests), and this is a project maintained by one person, so it seems reasonable to combine test cases to some degree as above. I just wonder if that's only delaying the issue, though, as we add more and more test code (whether fully new or modified)
cc: @msridhar for any thoughts on this.
There was a problem hiding this comment.
Thank you for the explanation and I agree this is not a good practice. We should definitely try to come up with a solution in future to resolve this. I have been planing to redesign test architecture (postponed to after submission deadline).
The main issue is that there are calls to build tasks in the test module that creates a series of .tsv files. This is the bottleneck of testing that makes the process heavy and slow. I was thinking to mock this but this (the call to subprocess that builds the target project) will require defining a series of mocked outputs that might making a new unit tests too difficult as it happens multiple times with different data for each unit tests.
|
@msridhar @lazaroclapp Thank you very much for the review. I will land this now. We can continue the discussion in #137 |
This PR fixes a bug which was causing annotator to only inject
@NullUnmarkedannotations on methods. This PR fixes this issue and enables Annotator to add@NullUnmarkedannotation for errors enclosed by constructors.