Add more tests to ASSERT_STATUS_CHECKED (5) #7737

adamretter · 2020-12-02T18:01:21Z

Fifth batch of adding more tests to ASSERT_STATUS_CHECKED.

db_test
db_test2

Requires #7819 for all tests to pass.

mrambacher

Thanks for all of the hard work in this PR. That is quite a few places to check.

Generally, it is fine. Questions on what to do with status in a thread and where to check the status in the iterator -- I prefer as one of the "last" things or after a Valid() check.

db/db_test.cc

mrambacher · 2020-12-30T01:47:24Z

db/db_test.cc

-  assert(versions);
-  assert(versions->GetColumnFamilySet());


I am not saying if these changes are right or wrong (I prefer the latter) but I hit issues with similar changes with TSAN/ASAN before I think.

Hmm... I just wanted things to be consistent. It seems strange to sometimes use assert and at other times the GTest ASSERT* statements.

These apparently useless non-nullity checks are hints for clang-analyze not to generate false reports. I don't believe clang-analyze understands GTest ASSERT*. (I always recommend "why" comments where justification is not clear.)

@pdillinger So should these be changed back? There are many changes like these in lots of tests - does that also then mean that we need to change these in all tests?

Consider the CircleCI build-linux-clang10-clang-analyze to be authoritative. If it's happy, we should be good. (Hopefully our internal clang-analyze runs will agree with it; we can worry about any disagreements between the two as they happen.)

db/db_test.cc

db/db_test2.cc

table/block_fetcher.h

trace_replay/trace_replay.cc

facebook-github-bot

@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

akankshamahajan15 · 2021-01-12T19:12:29Z

@adamretter DBTest.ThreadLocalPtrDeadlock is failing in ASAN check and ASSERT_STATUS_CHECKED.

akankshamahajan15 · 2021-02-26T01:41:40Z

@adamretter Hi Adam, were you able to resolve the failure?

akankshamahajan15 · 2021-06-21T22:58:16Z

@adamretter, Hi Adam. It's being a long time this PR has been in pending queue. Is there any update on this?

adamretter · 2021-06-22T20:58:49Z

Hi @akankshamahajan15 I rebased this PR this morning atop HEAD. I spent some time comparing and running the tests before and after rebase. Unfortunately after rebase (after resolving the merge conflicts) it now shows that there are several more failing tests than before, this is in part due to two things:

new tests have been added to db_test and db_test2 since this original PR, and those tests do not pass ASC.
changes have been made since this original PR, and those changes do not pass ASC (possibly because their tests were added to db_test and db_test2).

Is it best to push the rebased version of this PR and start resolving the new failures from here? Another option could be to split this PR into two, one for db_test and one for db_test2 as they are both quite large. What do you think? Let me know and I am happy to try and proceed.

p.s. the original issue DBTest.ThreadLocalPtrDeadlock failure still stands, but I could take another stab at it also.

akankshamahajan15 · 2021-06-22T21:08:57Z

Hi @akankshamahajan15 I rebased this PR this morning atop HEAD. I spent some time comparing and running the tests before and after rebase. Unfortunately after rebase (after resolving the merge conflicts) it now shows that there are several more failing tests than before, this is in part due to two things:

new tests have been added to db_test and db_test2 since this original PR, and those tests do not pass ASC.

changes have been made since this original PR, and those changes do not pass ASC (possibly because their tests were added to db_test and db_test2).

Is it best to push the rebased version of this PR and start resolving the new failures from here? Another option could be to split this PR into two, one for db_test and one for db_test2 as they are both quite large. What do you think? Let me know and I am happy to try and proceed.

p.s. the original issue DBTest.ThreadLocalPtrDeadlock failure still stands, but I could take another stab at it also.

Thanks a lot for taking it up. I think we If we can separate the two tests, we can atleast land one test otherwise more tests will keep getting added in future. Also we have techdebt week next week so let me know if you need any help. I can take a look at DBTest.ThreadLocalPtrDeadlock next week. It would be great if we can land this one.

mrambacher · 2021-06-23T12:47:30Z

Hi @akankshamahajan15 I rebased this PR this morning atop HEAD. I spent some time comparing and running the tests before and after rebase. Unfortunately after rebase (after resolving the merge conflicts) it now shows that there are several more failing tests than before, this is in part due to two things:

new tests have been added to db_test and db_test2 since this original PR, and those tests do not pass ASC.

changes have been made since this original PR, and those changes do not pass ASC (possibly because their tests were added to db_test and db_test2).

Is it best to push the rebased version of this PR and start resolving the new failures from here? Another option could be to split this PR into two, one for db_test and one for db_test2 as they are both quite large. What do you think? Let me know and I am happy to try and proceed.
p.s. the original issue DBTest.ThreadLocalPtrDeadlock failure still stands, but I could take another stab at it also.

Thanks a lot for taking it up. I think we If we can separate the two tests, we can atleast land one test otherwise more tests will keep getting added in future. Also we have techdebt week next week so let me know if you need any help. I can take a look at DBTest.ThreadLocalPtrDeadlock next week. It would be great if we can land this one.

I am also willing to look into these next week, but would rather not re-invent the wheel. If we can get whatever is passing into master (without the ASC-okay flag), that would be great.

Summary: This is the `db_test2` parts of #7737 reworked on the latest HEAD. Pull Request resolved: #8640 Reviewed By: akankshamahajan15 Differential Revision: D30303684 Pulled By: mrambacher fbshipit-source-id: 263e2f82d849bde4048b60aed8b31e7deed4706a

Summary: This is the `db_test2` parts of facebook/rocksdb#7737 reworked on the latest HEAD. Pull Request resolved: facebook/rocksdb#8640 Reviewed By: akankshamahajan15 Differential Revision: D30303684 Pulled By: mrambacher fbshipit-source-id: 263e2f82d849bde4048b60aed8b31e7deed4706a

facebook-github-bot added the CLA Signed label Dec 2, 2020

adamretter force-pushed the assert-status-checked-5 branch from 0ae67fe to 0a04e34 Compare December 29, 2020 17:57

adamretter requested review from pdillinger and mrambacher December 29, 2020 22:10

adamretter added the enhancement label Dec 29, 2020

adamretter marked this pull request as ready for review December 29, 2020 22:10

mrambacher reviewed Dec 30, 2020

View reviewed changes

adamretter force-pushed the assert-status-checked-5 branch from f16960c to 4042135 Compare December 30, 2020 15:56

adamretter force-pushed the assert-status-checked-5 branch 2 times, most recently from f3e58e0 to adc01cd Compare January 9, 2021 18:00

adamretter added 5 commits January 10, 2021 13:50

Add further tests to ASSERT_STATUS_CHECKED

bc08f3e

Address code review comments by @mrambracher

a227bb6

Enable formatting of Status in GTest output

fc4fb37

Do not call GTest ASSERT_* or EXPECT_* in threads

97d4565

Address code review comments by @mrambracher

bebd22f

adamretter force-pushed the assert-status-checked-5 branch from 28bd34d to bf01a6b Compare January 10, 2021 12:50

Attempt to fix issue with lambda capture on Windows VS

3df1201

adamretter force-pushed the assert-status-checked-5 branch from bf01a6b to 3df1201 Compare January 10, 2021 13:42

adamretter requested a review from akankshamahajan15 January 10, 2021 21:42

facebook-github-bot reviewed Jan 11, 2021

View reviewed changes

adamretter mentioned this pull request Aug 9, 2021

Add db_test2 to to ASSERT_STATUS_CHECKED #8640

Closed

sike-evolve mentioned this pull request Nov 29, 2021

Get DBTest passing Assert Status Checked #9231

Closed

facebook-github-bot closed this in 79f4a04 Dec 9, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more tests to ASSERT_STATUS_CHECKED (5) #7737

Add more tests to ASSERT_STATUS_CHECKED (5) #7737

adamretter commented Dec 2, 2020 •

edited

Loading

mrambacher left a comment

mrambacher Dec 30, 2020

adamretter Dec 30, 2020 •

edited

Loading

pdillinger Jan 4, 2021

adamretter Jan 6, 2021

pdillinger Jan 6, 2021

facebook-github-bot left a comment

akankshamahajan15 commented Jan 12, 2021

akankshamahajan15 commented Feb 26, 2021

akankshamahajan15 commented Jun 21, 2021

adamretter commented Jun 22, 2021 •

edited

Loading

akankshamahajan15 commented Jun 22, 2021

mrambacher commented Jun 23, 2021

Add more tests to ASSERT_STATUS_CHECKED (5) #7737

Add more tests to ASSERT_STATUS_CHECKED (5) #7737

Conversation

adamretter commented Dec 2, 2020 • edited Loading

mrambacher left a comment

Choose a reason for hiding this comment

mrambacher Dec 30, 2020

Choose a reason for hiding this comment

adamretter Dec 30, 2020 • edited Loading

Choose a reason for hiding this comment

pdillinger Jan 4, 2021

Choose a reason for hiding this comment

adamretter Jan 6, 2021

Choose a reason for hiding this comment

pdillinger Jan 6, 2021

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

akankshamahajan15 commented Jan 12, 2021

akankshamahajan15 commented Feb 26, 2021

akankshamahajan15 commented Jun 21, 2021

adamretter commented Jun 22, 2021 • edited Loading

akankshamahajan15 commented Jun 22, 2021

mrambacher commented Jun 23, 2021

adamretter commented Dec 2, 2020 •

edited

Loading

adamretter Dec 30, 2020 •

edited

Loading

adamretter commented Jun 22, 2021 •

edited

Loading