Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistently use override in tests #3934

Merged

Conversation

StephanTLavavej
Copy link
Member

As a general rule, we avoid significant cleanups of test code. This is partly because some tests are deliberately doing strange things to exercise weird corners of the product, and "cleaning up" the strangeness could affect test coverage. It's also because test code is much less important for long-term maintainability - if the tests keep passing, we generally don't care about them after they've been written.

However, we do try to enforce some basic conventions across the entire codebase including the tests. In the product code, we now follow a convention of saying only override. This avoids redundancy and allows virtual and override to serve different purposes (now when we see virtual we know we're introducing a new virtual member function). And using override is much safer than not using it.

This is so important for safety, and so non-disruptive, that it's worth making the tests consistent. (Including within the legacy tr1 test suite that we even more strongly avoid changing.) This PR consists of two commits performing related systematic changes:

  • Test style: Drop virtual when we override.
  • Test style: Use override instead of virtual.

This follows the product code convention, and is an extremely safe change that can't disturb test coverage.
This is sufficiently important for avoiding mistakes, and non-intrusive for test coverage, that I believe it's worth cleaning up `tr1` to be consistent with product code.
@StephanTLavavej StephanTLavavej added the test Related to test code label Aug 8, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner August 8, 2023 01:01
@StephanTLavavej StephanTLavavej mentioned this pull request Aug 8, 2023
@AlexGuteniev
Copy link
Contributor

Have you ran tests against Core Guidelines with only virtual rules ? `
See example from #2094 : https://godbolt.org/z/jfx9dzb1T

@StephanTLavavej
Copy link
Member Author

StephanTLavavej commented Aug 8, 2023

No - we are not yet attempting to be Core Guidelines clean. (There's a subset of important rules that the internal build will complain about.)

If you're asking whether I used the Core Guidelines to find occurrences, no - I just searched. I might have missed some, but this PR should be a strict improvement.

@AlexGuteniev
Copy link
Contributor

Sure I was asking just about few Core Guidelines which catch missing override and superfluous virtual / override

@StephanTLavavej StephanTLavavej self-assigned this Aug 10, 2023
@StephanTLavavej
Copy link
Member Author

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 8674b3d into microsoft:main Aug 11, 2023
37 checks passed
@StephanTLavavej StephanTLavavej deleted the let-override-let-overwrite branch August 11, 2023 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Related to test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants