mbaret commented on PR #12550: URL: https://github.com/apache/tvm/pull/12550#issuecomment-1243771649
@ekalda Thanks for taking another look and no worries for the delay. The addition of `CreateStorage` and the associated test was to address the concern from @Mousius raised here: > I don't think you can test all the behaviours of ExprAllocator through AOTMainLowerer due to cases such as: https://github.com/apache/tvm/pull/12550/files#diff-fd79a3a236f19a9f85e5045e7c75ed47a09ad7d698042114a93f4f74dd17c75eR99-R101 Which is an internal optimisation inside of ExprAllocator that hopefully we don't remove but makes no difference to the outer behaviour of the AOT pass and is therefore difficult to assert is behaving correctly. I can't write a test against `ExprAllocator` directly without exposing it through the header because otherwise I can't include it in the test case. That's why I added `CreateStorage`, to provide an exposed function through which to probe `ExprAllocator` (like how we have `AOTLowerMain` and `AOTMainLowerer`). To me this is slightly problematic because it was never the intent to expose `ExprAllocator`, but I have no choice if we require tests are written against it directly. The reason that particular test has been added is because it's one behaviour of `ExprAllocator` we can't test through the interface of `AOTLowerMain`. This is because `AOTLowerMain` will throw an exception if it encounters a nested function. However, `AOTLowerMain` will throw _after_ `ExprAllocator` runs, so the code is still in a strict sense reachable. > I'd test ExprAllocator in this case since besides being an implementation detail it is a chunky pass with complex logic. I think this is a fair point, and to be honest were I writing this from scratch I probably would have added some tests for `ExprAllocator` to help write it in the first place. In fact, if a bit more love and time were put into `ExprAllocator` it could probably be made into a reusable component (rather than an implementation detail), but I think that exceeds the scope of this refactor. However, now we also have `AOTLowerMain` written, to test `ExprAllocator` entirely separately would require a lot of duplicated tests (the `AOTLowerMain` tests would be looking at the `Allocate`s produced whereas the `ExprAllocator` tests would look at the `StorageInfo`s). If we want to spend this effort, my personal view is it should be done in the context of promoting `ExprAllocator` to be a proper analysis pass rather than as part of this refactor. > I also don't think the "ensure test coverage of the new feature" of the code quality guidelines is particularly helpful here since it is not defined what "test coverage" means. Agreed, and I think the fact there are 5 committers in this discussion all with differing interpretations of those guidelines is testament to that point 😅 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
