Page MenuHomePhabricator

Bug 1950424 part 7 - make Content Analysis gtests less race-y r=#dlp-reviewers!
ClosedPublic

Authored by gstoll on Mar 11 2025, 7:41 PM.
Referenced Files
Unknown Object (File)
Mon, Apr 13, 4:16 PM
Unknown Object (File)
Mon, Apr 13, 5:50 AM
Unknown Object (File)
Sat, Apr 11, 8:27 PM
Unknown Object (File)
Sat, Apr 11, 11:39 AM
Unknown Object (File)
Sat, Apr 11, 12:52 AM
Unknown Object (File)
Tue, Apr 7, 1:49 PM
Unknown Object (File)
Tue, Apr 7, 1:01 PM
Unknown Object (File)
Tue, Apr 7, 11:42 AM
Subscribers
None

Details

Summary

Adding the batch mode test exposed some other problems in the gtests.

There are a few categories of changes here:

  1. Wait for acknowledgments after sending a request and getting a response. This is helpful because sending an acknowledgment requires calling into the client, and this was often happening after the test was finished, which could cause the client to be recreated at weird times. Note that this isn't possible for all tests, but it is possible for most. This is also helpful in some cases because we weren't testing that setting autoAcknowledge (like all of our real use cases do right now) was actually causing acknowledgments to be sent.
  2. When waiting for acknowledgments, be tolerant to the fact that other operations might be going on, and just look for the request token that the code is interested in instead of asserting that the number of acknowledgements is what we expect.
  3. When killing the agent (for testing purposes) and starting the agent again, call SendSimpleRequestAndWaitForResponse() to force the client to be created before the end of the test and avoid issues like #1 above.

There are a few other very small things I'll flag as review comments.

Diff Detail

Event Timeline

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
toolkit/components/contentanalysis/tests/gtest/TestContentAnalysis.cpp
404

In this case we're cancelling and we don't really care about the acknowledgments, so suppress them to avoid causing problems for later tests.

838

I made these strings unique when running into Weird Test Problems(TM), and figured I might as well leave them unique.

1074

Here we weren't waiting for acknowledgments before, and it's also a good place to verify that the acknowledgment is correct.

address comments on patch 1

address handyman's comments

address handyman's comments on patches 4 and 6

This revision is now accepted and ready to land.Mar 13 2025, 8:43 PM