[SERVER-55246] Add SCons support for TCMalloc tests and validate they pass Created: 17/Mar/21 Updated: 06/Feb/24 Resolved: 06/Feb/24 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Tammy Bailey (Inactive) | Assignee: | Backlog - Service Architecture |
| Resolution: | Duplicate | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
||||||||
| Issue Links: |
|
||||||||
| Assigned Teams: |
Service Arch
|
||||||||
| Participants: | |||||||||
| Story Points: | 8 | ||||||||
| Description |
|
In SERVER-54747 we created a build with the new TCMalloc library but the tests were not included as part of the build. The following items should be completed as part of this ticket:
|
| Comments |
| Comment by Alison Felizzi (Inactive) [ 30/Mar/21 ] | |||||||||||
|
Hi acm , sorry about the confusion! I misinterpreted the meaning of a 'test registration' framework. I wasn't aware of the RegisterTest functionality in SCons (rather I incorrectly zeroed in on linking with mongo/unittest/unittest_main). What you have suggested is much cleaner! I've updated my branch with your suggestion. Thanks for the feedback | |||||||||||
| Comment by Andrew Morrow (Inactive) [ 30/Mar/21 ] | |||||||||||
|
Hi alison.felizzi - To be clear, my suggestion applied only to the SCons integration - not to switching tcmalloc away from using googletest. In some ways I wish that the mongo test framework really was googletest, rather than our hand rolled one. I agree with your decision not to alter the tcmalloc sources and to continue to use their test framework. However, I do feel pretty strongly that you would benefit from not re-implementing the existing in-build-system test execution facility as you have here: https://github.com/alisonfel/mongo/commit/4d34383be26b6881006c9f37599657ffe6ecac10#diff-197a920b4b71a216c0b726def0d4cc3d8cd1ce6f76b74e23a0374a3adce555f5R231 I think you could entirely make use of the facilities offered by the current build system to achieve this. There is no requirement that tests use the mongo test framework in order to participate in the build system level integrations. For instance the visibility tests: https://github.com/mongodb/mongo/blob/master/src/mongo/platform/SConscript#L89-L108 I think you could replace your existing create_tcmalloc_test with something like the following:
That would get you the + syntax for these tests automatically without needing to roll your own Command implementation, and would mean that your tests were automatically included among the unit tests on evergreen. And when using - | |||||||||||
| Comment by Alison Felizzi (Inactive) [ 30/Mar/21 ] | |||||||||||
|
Hey acm, thanks for the useful feedback and raising some great points! Its clear that it would be more ideal to use the existing test registration and execution facilities (over using googletest for test registration and the custom Alias scheme for execution). I took the aforementioned path as to avoid updating and patching the tcmalloc source. This being the path of least friction to mainly validate that the tests run and pass using the MongoDB toolchain and it's specific version of gcc 8.2 (being the main objective of this ticket). Also I was partially hesitant making significant tcmalloc source changes until the initial perf validations are completed. Beyond this initial version of compiling and running the tests, would using the existing test framework provide additional validation/insight to this ticket? Would be more than happy to iterate using our test framework if so! I believe the intention was that once the initial performance evaluation is completed it would be the point we start ensuring full/complete TCMalloc support throughout the codebase (Stage 3 of the Technical Design Doc). That would definitely be an ideal point to add complete integration with our server testing. | |||||||||||
| Comment by Andrew Morrow (Inactive) [ 29/Mar/21 ] | |||||||||||
|
alison.felizzi - I think it would be somewhat better if you integrated with the existing test registration and execution alias facilities that apply to the other unit tests in the server tree, rather than rolling your own execution framework and Alias scheme. That way, as we make improvements to the test execution facilities the tcmalloc tests will benefit as well. Please reach out on #server-build-help if you would like some thoughts on how to go about that. Another benefit is that if for some reason it isn't currently possible to do so, we can make improvements to the test execution facilities to make it possible. |