Mousius commented on PR #12550: URL: https://github.com/apache/tvm/pull/12550#issuecomment-1231464561
> > > I think it is reachable because we can always construct a test case with a function marked kPrimitive for AOTMainLowerer. If that's the primary concern here I'll happily add in such a test. I would caution though that the [TVM Code Quality Guidelines](https://github.com/apache/tvm/blob/main/docs/contribute/code_review.rst#factors-to-consider-about-code-quality) make no commitment to 100% coverage, so I think it's important we focus our testing effort wisely (I certainly haven't covered every corner case in the current tests). > > > > > > Yip, you can interpret "test coverage" differently, I'd rather not gamble on whether a future change will be caught or not by the tests though 😸 > > @Mousius regarding test coverage: generally I'm supportive of having more test coverage, but i'm not sure in this case it's necessary to block the PR over it; in this case, i think the regression is that we'd visit more of the main function than we'd need to in building `expr_storage_map_`. to me this falls a bit more on the "perfect" side of the don't-let-perfect-be-the-enemy-of-good argument. is there another kind of regression you can see here that is hard to test without explicitly testing ExprAllocator standalone? I've requested a minimal amount of additional tests be added to this PR, which is attempting to address a [broken window](https://en.wikipedia.org/wiki/Broken_windows_theory) in the code base. Please don't get hung up on the singular example I found from a brief review, what troubles me more is the active resistance to investing effort in testing, this is not good and far from perfect. If we are aware the behaviour could regress, and we can solidify the behaviour with a test case, I don't understand the rationale for ignoring it? We're already ignoring a large amount of the overall logic by not testing error cases, which in itself means we can't guarantee the component fails safely. -- 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]
