What is IsRequestActive() used for in the context of seeing whether there's an access token request? If it's just to check that there is a pending request, is it always followed by responding to the request? If so, we can just remove them as you wrote for (2).

This is an example:

434 TEST_F(UserPolicySigninServiceTest, InitRefreshTokenAvailableBeforeSignin) {
435   // Make sure user is not signed in.
436   ASSERT_FALSE(identity_test_env()->identity_manager()->HasPrimaryAccount());
437 
438   // No oauth access token yet, so client registration should be deferred.
439   ASSERT_FALSE(IsRequestActive());
440 
441   // Make oauth token available.
442   std::string account_id =
443       AccountTrackerServiceFactory::GetForProfile(profile_.get())
444           ->SeedAccountInfo(kTestGaiaId, kTestUser);
445   identity_test_env()->SetRefreshTokenForAccount(account_id);
446 
447   // Not signed in yet, so client registration should be deferred.
448   ASSERT_FALSE(IsRequestActive());
449 
450   // Sign in to Chrome.
451   identity_test_env()->MakePrimaryAccountAvailable(kTestUser);
452 
453   // Complete initialization of the store.
454   mock_store_->NotifyStoreLoaded();
455 
456   // Client registration should be in progress since we now have an oauth token
457   // for the authenticated account id.
458   EXPECT_EQ(mock_store_->signin_account_id(), test_account_id_);
459   ASSERT_TRUE(IsRequestActive());
460 }

IsRequestActive is used to ensure certain pre/post-conditions and specific states on the logic of the test/flow.

If you comment out lines 242-243, do any tests fail? i.e., can this be changed to just be a check of whether there are any requests pending on |test_url_loader_factory_|?

If there are failures, can you post them here? We can then narrow down our focus and figure out how to translate just those and then take out lines 242-243 (and maybe rename the method IsURLRequestActive() or something).

 Issue 911172  has been merged into this issue.

From  bug 911172  (by blundell):

It's just used to check that GetPendingRequests() isn't empty. Where WaitForAccessTokenRequest* is already used, we can just remove the check entirely, as WaitForAccessTokenRequest* waits for an access token request internally and thus implicitly checks that an access token request occurs. Where it's just used to verify that there's an access token request outstanding, we can replace the usage with a call to WaitForAccessTokenRequest*.

We can rename IsRequestActive() to IsNetworkRequestActive() and make sure that it's only used when we're verifying that there's a network request, not to verify that there's a pending access token request.

Don't spend too much time trying to get this exactly right, as in the end this is not the fundamentals of this test code. We can always refine the changes during code review.

(re comment #4)

If I comment out lines 242-243, the following tests fail:

$ <out>/unit_tests --gtest_filter=UserPolicySigninServiceTest.InitRefreshTokenAvailableBeforeSignin
(...)
UserPolicySigninServiceTest.InitRefreshTokenAvailableBeforeSignin
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from UserPolicySigninServiceTest
[ RUN      ] UserPolicySigninServiceTest.InitRefreshTokenAvailableBeforeSignin
../../chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:459: Failure
Value of: IsRequestActive()
  Actual: false
Expected: true
Stack trace:
#0 0x559e45d3673c testing::internal::UnitTestImpl::CurrentOsStackTraceExceptTop()
#1 0x559e45d36119 testing::internal::AssertHelper::operator=()
#2 0x559e44857690 policy::(anonymous namespace)::UserPolicySigninServiceTest_InitRefreshTokenAvailableBeforeSignin_Test::TestBody()

[  FAILED  ] UserPolicySigninServiceTest.InitRefreshTokenAvailableBeforeSignin (26 ms)
[----------] 1 test from UserPolicySigninServiceTest (26 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (27 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] UserPolicySigninServiceTest.InitRefreshTokenAvailableBeforeSignin

 1 FAILED TEST
[20432:20436:1220/131755.936094:188141512729:ERROR:kill_posix.cc(84)] Unable to terminate process group 20437: No such process (3)
[1/1] UserPolicySigninServiceTest.InitRefreshTokenAvailableBeforeSignin (26 ms)
1 test failed:
    UserPolicySigninServiceTest.InitRefreshTokenAvailableBeforeSignin (../../chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:434)
Tests took 0 seconds.




$ <out>/unit_tests --gtest_filter=UserPolicySigninServiceTest.SignInAfterInit

IMPORTANT DEBUGGING NOTE: batches of tests are run inside their
own process. For debugging a test inside a debugger, use the
--gtest_filter=<your_test_name> flag along with
--single-process-tests.
Using sharding settings from environment. This is shard 0/1
Using 1 parallel jobs.

Note: Google Test filter = UserPolicySigninServiceTest.SignInAfterInit
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from UserPolicySigninServiceTest
[ RUN      ] UserPolicySigninServiceTest.SignInAfterInit
../../chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:529: Failure
Value of: IsRequestActive()
  Actual: false
Expected: true
Stack trace:
#0 0x563bd58e073c testing::internal::UnitTestImpl::CurrentOsStackTraceExceptTop()
#1 0x563bd58e0119 testing::internal::AssertHelper::operator=()
#2 0x563bd4402585 policy::(anonymous namespace)::UserPolicySigninServiceTest_SignInAfterInit_Test::TestBody()

[  FAILED  ] UserPolicySigninServiceTest.SignInAfterInit (20 ms)
[----------] 1 test from UserPolicySigninServiceTest (20 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (20 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] UserPolicySigninServiceTest.SignInAfterInit

 1 FAILED TEST




$ <out>/unit_tests --gtest_filter=UserPolicySigninServiceTest.UnregisteredClient
IMPORTANT DEBUGGING NOTE: batches of tests are run inside their
own process. For debugging a test inside a debugger, use the
--gtest_filter=<your_test_name> flag along with
--single-process-tests.
Using sharding settings from environment. This is shard 0/1
Using 1 parallel jobs.
Note: Google Test filter = UserPolicySigninServiceTest.UnregisteredClient
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from UserPolicySigninServiceTest
[ RUN      ] UserPolicySigninServiceTest.UnregisteredClient
../../chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:575: Failure
Value of: IsRequestActive()
  Actual: false
Expected: true
Stack trace:
#0 0x557dfcdc873c testing::internal::UnitTestImpl::CurrentOsStackTraceExceptTop()
#1 0x557dfcdc8119 testing::internal::AssertHelper::operator=()
#2 0x557dfb8eacee policy::(anonymous namespace)::UserPolicySigninServiceTest_UnregisteredClient_Test::TestBody()

[  FAILED  ] UserPolicySigninServiceTest.UnregisteredClient (20 ms)
[----------] 1 test from UserPolicySigninServiceTest (20 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (20 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] UserPolicySigninServiceTest.UnregisteredClient

 1 FAILED TEST
[21076:21081:1220/132445.410644:188550987307:ERROR:kill_posix.cc(84)] Unable to terminate process group 21082: No such process (3)
[1/1] UserPolicySigninServiceTest.UnregisteredClient (20 ms)
1 test failed:
    UserPolicySigninServiceTest.UnregisteredClient (../../chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc:552)
Tests took 0 seconds.

Thanks!

First failure: We should just be able to replace with a call to WaitForAccessTokenRequest..., since it's just checking that an access token request was made in response to the refresh token being made available.

Second failure: Likewise.

Third failure: Likewise. (All of the "Client registration should be in progress since we have an oauth token" are giveaways).