https://github.com/ilovepi commented:
Thanks for adding more tests. Clang-Doc badly needs more comprehensive test coverage. I have a few concerns, however, about the testing strategy. Largely, these tests seem to check the exact output, rather than spot checking for particular features. What I mean by that is it seems like these tests are expected to be updated any time we modify clang-doc. There is a certain amount of value in having a few tests that track format changes, but I'm not too sure we want all of our tests to be of that style. For instance, do we need to check the JS output so precisely? It's good that we'll know the JS output has changed to a certain degree, but I'm not sure that *any* change is necessarily *wrong*. So I think it would be good to go through and minimally test for useful output changes, without precisely matching all of the boiler plate HTML. It's also worth noting that the Markdown is much more concise, and seems to mostly contain the documentation text. We may want to consider testing all the useful output properties w/ Markdown output, but maintain a few tests that verify the HTML formatting and JS bits. That type of strategy is pretty common in LLVM. Lastly, it seems like this patch is adding a number to different independent tests. It probably makes more sense to add them independently. I'd say at the least, the projects should be split out into independent patches, as well as the 2 standalone tests. https://github.com/llvm/llvm-project/pull/97518 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
