ucr-riple / NullAwayAnnotator

A tool to help adapting code bases to NullAway type system.
MIT License
13 stars 6 forks source link

Redesign test architecture #137

Open nimakarimipour opened 1 year ago

nimakarimipour commented 1 year ago

Describe the task

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.

nimakarimipour commented 1 year ago

One solution is to keep CI light for new features that we would like to test if:

  1. The code pattern is very small (in this case just a constructor)
  2. The code pattern is not so different from other existing tests.
  3. The selected unit tests to be embedded is testing the similar behavior with new feature.
  4. 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.

nimakarimipour commented 1 year ago

Comment from @lazaroclapp:

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.

nimakarimipour commented 1 year ago

@lazaroclapp 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.

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 I created the issue so we can continue the discussion over this thread and land #133.