shimless_rma: Use TestUserSessionManager [chromium/src : main]
Achuith Bhandarkar (Gerrit)
Achuith Bhandarkar added 1 comment![]( "Open in Gerrit")
Split out from large CL. PTAL, Hidehiko-san
Related details
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 2 comments![]( "Open in Gerrit")
: user_session_manager_(as you're resetting this in TearDown(), could you set it up in SetUp() to keep the consistency?
// Destroy UserSessionManager after SessionManager (inside TearDown) toRelated details
- Achuith Bhandarkar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Achuith Bhandarkar (Gerrit)
Achuith Bhandarkar added 2 comments![]( "Open in Gerrit")
: user_session_manager_(as you're resetting this in TearDown(), could you set it up in SetUp() to keep the consistency?
The problem is that AshTestHelper also creates a SessionManager if one doesn't exist (NoSessionAshTestBase::SetUp() calls AshTestBase::SetUp(), which instantiates AshTestHelper), so we have to do this early. I can add a comment here to explain this unless there's some other way to approach this.
// Destroy UserSessionManager after SessionManager (inside TearDown) toditto
https://chromium-review.git.corp.google.com/c/chromium/src/+/8027383/comment/3b0c24bb_60acb22a/
This is the problem:
https://source.chromium.org/chromium/chromium/src/+/main:ash/test/ash_test_helper.cc;l=132-137
So AshTestHelper can touch the sessionmanager, so we should destroy the user_session_manager_ last. (I've updated the comment to be more clear).
We don't have to do it this way because the internal session_manager unique pointer in AshTestHelper is set to null so it should not be a problem.
Related details
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 2 comments![]( "Open in Gerrit")
: user_session_manager_(Achuith Bhandarkaras you're resetting this in TearDown(), could you set it up in SetUp() to keep the consistency?
The problem is that AshTestHelper also creates a SessionManager if one doesn't exist (NoSessionAshTestBase::SetUp() calls AshTestBase::SetUp(), which instantiates AshTestHelper), so we have to do this early. I can add a comment here to explain this unless there's some other way to approach this.
Also, you can still put the initialization statement in SetUp() of this class, but still earlier than NoSessionAShTestBase::SetUp().
// Destroy UserSessionManager after SessionManager (inside TearDown) toAchuith Bhandarkarditto
https://chromium-review.git.corp.google.com/c/chromium/src/+/8027383/comment/3b0c24bb_60acb22a/
This is the problem:
https://source.chromium.org/chromium/chromium/src/+/main:ash/test/ash_test_helper.cc;l=132-137So AshTestHelper can touch the sessionmanager, so we should destroy the user_session_manager_ last. (I've updated the comment to be more clear).
We don't have to do it this way because the internal session_manager unique pointer in AshTestHelper is set to null so it should not be a problem.
- Achuith Bhandarkar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Achuith Bhandarkar (Gerrit)
Achuith Bhandarkar added 5 comments![]( "Open in Gerrit")
: user_session_manager_(Achuith Bhandarkaras you're resetting this in TearDown(), could you set it up in SetUp() to keep the consistency?
Hidehiko AbeThe problem is that AshTestHelper also creates a SessionManager if one doesn't exist (NoSessionAshTestBase::SetUp() calls AshTestBase::SetUp(), which instantiates AshTestHelper), so we have to do this early. I can add a comment here to explain this unless there's some other way to approach this.
Also, you can still put the initialization statement in SetUp() of this class, but still earlier than NoSessionAShTestBase::SetUp().
Done
FakeRmadClientForTest::Initialize();DBUS services should be initialized first
user_session_manager_.reset();This should be in reverse order.
// Destroy UserSessionManager after SessionManager (inside TearDown) toAchuith Bhandarkarditto
https://chromium-review.git.corp.google.com/c/chromium/src/+/8027383/comment/3b0c24bb_60acb22a/
Hidehiko AbeThis is the problem:
https://source.chromium.org/chromium/chromium/src/+/main:ash/test/ash_test_helper.cc;l=132-137So AshTestHelper can touch the sessionmanager, so we should destroy the user_session_manager_ last. (I've updated the comment to be more clear).
We don't have to do it this way because the internal session_manager unique pointer in AshTestHelper is set to null so it should not be a problem.
PTAL: https://chromium-review.git.corp.google.com/c/chromium/src/+/8027383/comment/edda6be9_536b41a6/
Ok, I have tried to initialize services in SetUp to match prod, and destruct in reverse order in TearDown.
Related details
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe voted Code-Review+1![]( "Open in Gerrit")
- Achuith Bhandarkar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Achuith Bhandarkar (Gerrit)
Achuith Bhandarkar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Achuith Bhandarkar
| Commit-Queue | +2 |
1 comment
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
shimless_rma: Use TestUserSessionManager
Replaces FakeUserManager and ScopedUserManager with
TestUserSessionManager to manage user sessions in
ShimlessRmaServiceTest.
- M ash/webui/shimless_rma/DEPS
- M ash/webui/shimless_rma/backend/BUILD.gn
- M ash/webui/shimless_rma/backend/shimless_rma_service_unittest.cc
Code-Review: +1 by Hidehiko Abe
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |