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

Removed extra validation in flaky DataGridView unit tests. #8574

Conversation

dkazennov
Copy link
Contributor

@dkazennov dkazennov commented Feb 6, 2023

Fixes #7799 (the main Issue containing a complete list of flaky tests)
Fixes #6597
Fixes #6739
Fixes #6926 (along with #8535)

Proposed changes

  • This fix omits check for invalidatedCallCount in unstable unit tests with a commentary reference to this Issue to keep a track record (as proposed by @RussKie).
  • Theory data objects/inline data sets changed as well.

Tests changed:

  • DataGridView_ColumnHeadersHeightSizeMode_SetNonResizeThenResize_RestoresOldValue
  • DataGridView_ColumnHeadersHeightSizeMode_SetWithHandle_GetReturnsExpected
  • DataGridView_ColumnHeadersHeight_SetWithHandle_GetReturnsExpected
  • DataGridView_ColumnHeadersHeight_SetWithParentWithHandle_GetReturnsExpected
  • DataGridView_OnColumnHeadersHeightChanged_InvokeWithHandle_CallsColumnHeadersHeightChanged
  • DataGridView_OnColumnHeadersHeightSizeModeChanged_InvokeWithHandle_CallsColumnHeadersHeightSizeModeChanged
  • DataGridView_OnRowHeadersWidthChanged_InvokeWithHandle_CallsRowHeadersWidthChanged
  • DataGridView_OnRowHeadersWidthSizeModeChanged_InvokeWithHandle_CallsRowHeadersWidthSizeModeChanged
  • DataGridView_Parent_SetWithHandle_GetReturnsExpected
  • DataGridView_RowHeadersWidthSizeMode_SetNonResizeThenResize_RestoresOldValue
  • DataGridView_RowHeadersWidthSizeMode_SetWithHandle_GetReturnsExpected
  • DataGridView_RowHeadersWidth_SetWithHandle_GetReturnsExpected
  • DataGridView_RowHeadersWidth_SetWithParentWithHandle_GetReturnsExpected
  • DataGridView_TopLeftHeaderCell_GetWithHandle_ReturnsExpected
  • DataGridView_TopLeftHeaderCell_SetWithHandle_GetReturnsExpected

Customer Impact

  • No impact. This fix affects only unit tests.

Regression?

  • No

Risk

  • Minimal

Test methodology

  • Unit tests.

Test environment(s)

  • dotnet 8.0.100-alpha.1.22607.6
Microsoft Reviewers: Open in CodeFlow

@dkazennov dkazennov requested a review from a team as a code owner February 6, 2023 15:41
@ghost ghost assigned dkazennov Feb 6, 2023
@dkazennov dkazennov added the test-bug Problem in test source code (most likely) label Feb 6, 2023
@dkazennov dkazennov force-pushed the Issue_7799_Remove_ExtraInvalidation_inFlakyTests branch from ea1f263 to 65cfd58 Compare February 6, 2023 18:34
@Tanya-Solyanik
Copy link
Member

@dreddy-work - how do you feel about having commented out code in tests? I'm against it because the issue is already documenting the problem. I would remove the unused code and data and keep the reference to the issue.

@Tanya-Solyanik Tanya-Solyanik changed the title Removed extra validation in flacky DataGridView unit tests. Removed extra validation in flaky DataGridView unit tests. Feb 7, 2023
@dkazennov dkazennov force-pushed the Issue_7799_Remove_ExtraInvalidation_inFlakyTests branch from 65cfd58 to 6cbbccd Compare February 8, 2023 12:01
@dkazennov
Copy link
Contributor Author

@Tanya-Solyanik @dreddy-work

Given @merriemcgaw opinion here I've decided we'd better get rid of commented out code here (and change tests theories subsequently).

@kirsan31
Copy link
Contributor

kirsan31 commented Feb 8, 2023

I have nothing against it, but such an approach means that these tests will never be returned :)

@dkazennov dkazennov force-pushed the Issue_7799_Remove_ExtraInvalidation_inFlakyTests branch from 6cbbccd to ea4765d Compare February 8, 2023 12:29
@dkazennov
Copy link
Contributor Author

I have nothing against it, but such an approach means that these tests will never be returned :)

But we will still have this issue and all changes recorded, aren't we?

Copy link
Member

@Tanya-Solyanik Tanya-Solyanik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@dreddy-work dreddy-work enabled auto-merge (squash) February 8, 2023 17:39
Copy link
Contributor

@dmitrii-drobotov dmitrii-drobotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Perhaps we should keep the issue #7799 open until we fix the underlying issue with these tests and then bring back removed checks?

@Tanya-Solyanik
Copy link
Member

Perhaps we should keep the issue #7799 open until we fix the underlying issue with these tests and then bring back removed checks?

Makes sense. Please mark the issue appropriately, or better - open a new one dedicated to the Invalidate case.

@kirsan31
Copy link
Contributor

kirsan31 commented Feb 8, 2023

@Tanya-Solyanik

Please mark the issue appropriately, or better - open a new one dedicated to the Invalidate case.

Already here. 🫡 This PR not fix it (opposite to described in 1 post).

@Tanya-Solyanik
Copy link
Member

Already #8535. 🫡 This PR not fix it (opposite to described in 1 post).

Thank you! I suggest keeping #8535 for the functional fix later and close #6926 because we want to see the new failure counts after Invalidate is removed.

@dreddy-work dreddy-work merged commit 4ff37d7 into dotnet:main Feb 9, 2023
@ghost ghost added this to the 8.0 Preview2 milestone Feb 9, 2023
@ghost ghost removed the waiting-review This item is waiting on review by one or more members of team label Feb 9, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
test-bug Problem in test source code (most likely)
Projects
None yet
5 participants