Trace Redaction - Move Redactor+Context out of fixture
The integration test fixture's core responsibility is to manage source
trace and redacted trace files. Some common values we add to it to
avoid declaring them in every test (redactor and context).
This coupling was okay when each test populated the redactor. However,
we'll be adding tests that use the production config and having this
silent default value has already caused issued when not reset.
To avoid this easy mistakes, this removes the redactor and context
from the fixture and requires each test to provide their own.
Change-Id: Ica0e0752bf2f092a914f0d3bc262242615ec0bd4
diff --git a/src/trace_redaction/boardphase_packet_filter_integrationtest.cc b/src/trace_redaction/boardphase_packet_filter_integrationtest.cc
index 7c3d2fc..b72c84d 100644
--- a/src/trace_redaction/boardphase_packet_filter_integrationtest.cc
+++ b/src/trace_redaction/boardphase_packet_filter_integrationtest.cc
@@ -32,8 +32,8 @@
protected TraceRedactionIntegrationFixure {
protected:
void SetUp() override {
- trace_redactor()->emplace_build<PopulateAllowlists>();
- trace_redactor()->emplace_transform<BroadphasePacketFilter>();
+ trace_redactor_.emplace_build<PopulateAllowlists>();
+ trace_redactor_.emplace_transform<BroadphasePacketFilter>();
}
Context::TracePacketMask ScanPacketFields(const std::string& trace) {
@@ -70,6 +70,9 @@
return mask;
}
+ Context context_;
+ TraceRedactor trace_redactor_;
+
private:
Context::FtraceEventMask CopyEventFields(protozero::ConstBytes bytes) {
protozero::ProtoDecoder decoder(bytes);
@@ -90,12 +93,12 @@
// through redaction and checks that no excluded fields passed through
// redaction.
TEST_F(BroadphasePacketFilterIntegrationTest, OnlyKeepsIncludedPacketFields) {
- ASSERT_OK(Redact());
+ ASSERT_OK(Redact(trace_redactor_, &context_));
auto trace = LoadRedacted();
ASSERT_OK(trace);
- auto include_mask = context()->packet_mask;
+ auto include_mask = context_.packet_mask;
auto exclude_mask = ~include_mask;
auto fields = ScanPacketFields(*trace);
@@ -112,12 +115,12 @@
// redaction.
TEST_F(BroadphasePacketFilterIntegrationTest,
OnlyKeepsIncludedFtraceEventFields) {
- ASSERT_OK(Redact());
+ ASSERT_OK(Redact(trace_redactor_, &context_));
auto trace = LoadRedacted();
ASSERT_OK(trace);
- auto include_mask = context()->ftrace_mask;
+ auto include_mask = context_.ftrace_mask;
auto exclude_mask = ~include_mask;
auto fields = ScanFtraceEventFields(*trace);
diff --git a/src/trace_redaction/filter_sched_waking_events_integrationtest.cc b/src/trace_redaction/filter_sched_waking_events_integrationtest.cc
index abc6577..efd91c4 100644
--- a/src/trace_redaction/filter_sched_waking_events_integrationtest.cc
+++ b/src/trace_redaction/filter_sched_waking_events_integrationtest.cc
@@ -46,16 +46,19 @@
protected TraceRedactionIntegrationFixure {
protected:
void SetUp() override {
- trace_redactor()->emplace_collect<FindPackageUid>();
- trace_redactor()->emplace_collect<CollectTimelineEvents>();
+ trace_redactor_.emplace_collect<FindPackageUid>();
+ trace_redactor_.emplace_collect<CollectTimelineEvents>();
auto* redact_sched_events =
- trace_redactor()->emplace_transform<RedactSchedEvents>();
+ trace_redactor_.emplace_transform<RedactSchedEvents>();
redact_sched_events->emplace_modifier<ClearComms>();
redact_sched_events->emplace_waking_filter<ConnectedToPackage>();
- context()->package_name = kPackageName;
+ context_.package_name = kPackageName;
}
+
+ Context context_;
+ TraceRedactor trace_redactor_;
};
// >>> SELECT uid
@@ -110,7 +113,7 @@
// +------+-----------------+
TEST_F(RedactSchedWakingIntegrationTest, OnlyKeepsPackageEvents) {
- auto result = Redact();
+ auto result = Redact(trace_redactor_, &context_);
ASSERT_OK(result) << result.c_message();
auto original = LoadOriginal();
diff --git a/src/trace_redaction/filter_task_rename_integrationtest.cc b/src/trace_redaction/filter_task_rename_integrationtest.cc
index e2675e2..332bb2e 100644
--- a/src/trace_redaction/filter_task_rename_integrationtest.cc
+++ b/src/trace_redaction/filter_task_rename_integrationtest.cc
@@ -41,16 +41,16 @@
void SetUp() override {
// In order for ScrubTaskRename to work, it needs the timeline. All
// registered primitives are there to generate the timeline.
- trace_redactor()->emplace_collect<FindPackageUid>();
- trace_redactor()->emplace_collect<CollectTimelineEvents>();
+ trace_redactor_.emplace_collect<FindPackageUid>();
+ trace_redactor_.emplace_collect<CollectTimelineEvents>();
// Configure the system to drop every rename event not connected to the
// package.
- auto* redact = trace_redactor()->emplace_transform<RedactProcessEvents>();
+ auto* redact = trace_redactor_.emplace_transform<RedactProcessEvents>();
redact->emplace_filter<ConnectedToPackage>();
redact->emplace_modifier<DoNothing>();
- context()->package_name = "com.Unity.com.unity.multiplayer.samples.coop";
+ context_.package_name = "com.Unity.com.unity.multiplayer.samples.coop";
}
void GetRenamedPids(
@@ -82,10 +82,13 @@
return renamed_pids;
}
+
+ Context context_;
+ TraceRedactor trace_redactor_;
};
TEST_F(RenameEventsTraceRedactorIntegrationTest, RemovesUnwantedRenameTasks) {
- ASSERT_OK(Redact());
+ ASSERT_OK(Redact(trace_redactor_, &context_));
auto original = LoadOriginal();
ASSERT_OK(original);
diff --git a/src/trace_redaction/process_thread_timeline_integrationtest.cc b/src/trace_redaction/process_thread_timeline_integrationtest.cc
index ea95170..d88e2c3 100644
--- a/src/trace_redaction/process_thread_timeline_integrationtest.cc
+++ b/src/trace_redaction/process_thread_timeline_integrationtest.cc
@@ -38,12 +38,16 @@
protected TraceRedactionIntegrationFixure {
protected:
void SetUp() override {
- context()->package_name = "com.Unity.com.unity.multiplayer.samples.coop";
- trace_redactor()->emplace_collect<FindPackageUid>();
- trace_redactor()->emplace_collect<CollectTimelineEvents>();
+ context_.package_name = "com.Unity.com.unity.multiplayer.samples.coop";
- ASSERT_OK(Redact());
+ trace_redactor_.emplace_collect<FindPackageUid>();
+ trace_redactor_.emplace_collect<CollectTimelineEvents>();
+
+ ASSERT_OK(Redact(trace_redactor_, &context_));
}
+
+ Context context_;
+ TraceRedactor trace_redactor_;
};
TEST_F(ProcessThreadTimelineIntegrationTest, PackageThreadsAreConnected) {
@@ -63,8 +67,8 @@
for (auto pid : threads) {
// Use EXPECT instead of ASSERT to test all values.
- EXPECT_TRUE(context()->timeline->PidConnectsToUid(kTime, pid,
- *context()->package_uid));
+ EXPECT_TRUE(
+ context_.timeline->PidConnectsToUid(kTime, pid, *context_.package_uid));
}
}
@@ -73,8 +77,8 @@
// select uid from package_list where
// package_name='com.Unity.com.unity.multiplayer.samples.coop')
- ASSERT_TRUE(context()->timeline->PidConnectsToUid(kTime, 7105,
- *context()->package_uid));
+ ASSERT_TRUE(
+ context_.timeline->PidConnectsToUid(kTime, 7105, *context_.package_uid));
}
TEST_F(ProcessThreadTimelineIntegrationTest,
@@ -96,8 +100,8 @@
for (auto pid : threads) {
// Use EXPECT instead of ASSERT to test all values.
- EXPECT_FALSE(context()->timeline->PidConnectsToUid(
- kTime, pid, *context()->package_uid));
+ EXPECT_FALSE(
+ context_.timeline->PidConnectsToUid(kTime, pid, *context_.package_uid));
}
}
diff --git a/src/trace_redaction/prune_package_list_integrationtest.cc b/src/trace_redaction/prune_package_list_integrationtest.cc
index 04ec15c..9f98c29 100644
--- a/src/trace_redaction/prune_package_list_integrationtest.cc
+++ b/src/trace_redaction/prune_package_list_integrationtest.cc
@@ -51,10 +51,10 @@
protected TraceRedactionIntegrationFixure {
protected:
void SetUp() override {
- context()->package_name = kPackageName;
+ context_.package_name = kPackageName;
- trace_redactor()->emplace_collect<FindPackageUid>();
- trace_redactor()->emplace_transform<PrunePackageList>();
+ trace_redactor_.emplace_collect<FindPackageUid>();
+ trace_redactor_.emplace_transform<PrunePackageList>();
}
std::vector<protos::gen::PackagesList::PackageInfo> GetPackageInfo(
@@ -91,6 +91,9 @@
return names;
}
+
+ Context context_;
+ TraceRedactor trace_redactor_;
};
// It is possible for two packages_list to appear in the trace. The
@@ -99,14 +102,14 @@
// packages_list to contain copies of each other - for example
// "com.Unity.com.unity.multiplayer.samples.coop" appears in both packages_list.
TEST_F(PrunePackageListIntegrationTest, FindsPackageAndFiltersPackageList) {
- auto result = Redact();
+ auto result = Redact(trace_redactor_, &context_);
ASSERT_OK(result) << result.message();
auto after_raw_trace = LoadRedacted();
ASSERT_OK(after_raw_trace) << after_raw_trace.status().message();
- ASSERT_TRUE(context()->package_uid.has_value());
- ASSERT_EQ(*context()->package_uid, kPackageUid);
+ ASSERT_TRUE(context_.package_uid.has_value());
+ ASSERT_EQ(*context_.package_uid, kPackageUid);
protos::pbzero::Trace::Decoder redacted_trace(after_raw_trace.value());
auto packages = GetPackageInfo(redacted_trace);
@@ -127,9 +130,9 @@
// the package list, so there is no way to differentiate these packages (only
// the uid is used later), so each entry should remain.
TEST_F(PrunePackageListIntegrationTest, RetainsAllInstancesOfUid) {
- context()->package_name = "com.google.android.networkstack.tethering";
+ context_.package_name = "com.google.android.networkstack.tethering";
- auto result = Redact();
+ auto result = Redact(trace_redactor_, &context_);
ASSERT_OK(result) << result.message();
auto after_raw_trace = LoadRedacted();
diff --git a/src/trace_redaction/redact_process_trees_integrationtest.cc b/src/trace_redaction/redact_process_trees_integrationtest.cc
index 9178c02..716cabc 100644
--- a/src/trace_redaction/redact_process_trees_integrationtest.cc
+++ b/src/trace_redaction/redact_process_trees_integrationtest.cc
@@ -45,21 +45,21 @@
protected TraceRedactionIntegrationFixure {
protected:
void SetUp() override {
- trace_redactor()->emplace_collect<CollectSystemInfo>();
- trace_redactor()->emplace_build<BuildSyntheticThreads>();
+ trace_redactor_.emplace_collect<CollectSystemInfo>();
+ trace_redactor_.emplace_build<BuildSyntheticThreads>();
- trace_redactor()->emplace_collect<FindPackageUid>();
- trace_redactor()->emplace_collect<CollectTimelineEvents>();
+ trace_redactor_.emplace_collect<FindPackageUid>();
+ trace_redactor_.emplace_collect<CollectTimelineEvents>();
// Filter the process tree based on whether or not a process is part of the
// target package.
auto* process_tree =
- trace_redactor()->emplace_transform<RedactProcessTrees>();
+ trace_redactor_.emplace_transform<RedactProcessTrees>();
process_tree->emplace_modifier<ProcessTreeDoNothing>();
process_tree->emplace_filter<ConnectedToPackage>();
// In this case, the process and package have the same name.
- context()->package_name = kProcessName;
+ context_.package_name = kProcessName;
}
std::unordered_set<int32_t> GetPids(const std::string& bytes) const {
@@ -94,6 +94,9 @@
return tids;
}
+ Context context_;
+ TraceRedactor trace_redactor_;
+
private:
void GetPids(protozero::ConstBytes bytes,
std::unordered_set<int32_t>* pids) const {
@@ -119,7 +122,7 @@
};
TEST_F(RedactProcessTreesIntegrationTest, FilterProcesses) {
- ASSERT_OK(Redact());
+ ASSERT_OK(Redact(trace_redactor_, &context_));
auto original_trace_str = LoadOriginal();
ASSERT_OK(original_trace_str);
@@ -151,7 +154,7 @@
}
TEST_F(RedactProcessTreesIntegrationTest, FilterThreads) {
- ASSERT_OK(Redact());
+ ASSERT_OK(Redact(trace_redactor_, &context_));
auto original_trace_str = LoadOriginal();
ASSERT_OK(original_trace_str);
@@ -185,19 +188,18 @@
TEST_F(RedactProcessTreesIntegrationTest, AddSynthProcess) {
// Append another primitive that won't filter, but will add new threads. This
// will be compatible with the other instanced in SetUp().
- auto* process_tree =
- trace_redactor()->emplace_transform<RedactProcessTrees>();
+ auto* process_tree = trace_redactor_.emplace_transform<RedactProcessTrees>();
process_tree->emplace_modifier<ProcessTreeCreateSynthThreads>();
process_tree->emplace_filter<AllowAll>();
- ASSERT_OK(Redact());
+ ASSERT_OK(Redact(trace_redactor_, &context_));
auto redacted_trace_str = LoadRedacted();
ASSERT_OK(redacted_trace_str);
auto redacted_pids = GetPids(*redacted_trace_str);
- const auto* synth_process = context()->synthetic_process.get();
+ const auto* synth_process = context_.synthetic_process.get();
ASSERT_TRUE(synth_process);
ASSERT_NE(std::find(redacted_pids.begin(), redacted_pids.end(),
@@ -208,14 +210,13 @@
TEST_F(RedactProcessTreesIntegrationTest, AddSynthThreads) {
// Append another primitive that won't filter, but will add new threads. This
// will be compatible with the other instanced in SetUp().
- auto* process_tree =
- trace_redactor()->emplace_transform<RedactProcessTrees>();
+ auto* process_tree = trace_redactor_.emplace_transform<RedactProcessTrees>();
process_tree->emplace_modifier<ProcessTreeCreateSynthThreads>();
process_tree->emplace_filter<AllowAll>();
- ASSERT_OK(Redact());
+ ASSERT_OK(Redact(trace_redactor_, &context_));
- const auto* synth_process = context()->synthetic_process.get();
+ const auto* synth_process = context_.synthetic_process.get();
ASSERT_TRUE(synth_process);
ASSERT_FALSE(synth_process->tids().empty());
diff --git a/src/trace_redaction/redact_sched_events_integrationtest.cc b/src/trace_redaction/redact_sched_events_integrationtest.cc
index f7b32cd..bf47328 100644
--- a/src/trace_redaction/redact_sched_events_integrationtest.cc
+++ b/src/trace_redaction/redact_sched_events_integrationtest.cc
@@ -91,15 +91,15 @@
protected TraceRedactionIntegrationFixure {
protected:
void SetUp() override {
- trace_redactor()->emplace_collect<FindPackageUid>();
- trace_redactor()->emplace_collect<CollectTimelineEvents>();
+ trace_redactor_.emplace_collect<FindPackageUid>();
+ trace_redactor_.emplace_collect<CollectTimelineEvents>();
auto* redact_sched_events =
- trace_redactor()->emplace_transform<RedactSchedEvents>();
+ trace_redactor_.emplace_transform<RedactSchedEvents>();
redact_sched_events->emplace_modifier<ClearComms>();
redact_sched_events->emplace_waking_filter<AllowAll>();
- context()->package_name = "com.Unity.com.unity.multiplayer.samples.coop";
+ context_.package_name = "com.Unity.com.unity.multiplayer.samples.coop";
}
std::unordered_map<int32_t, std::string> expected_names_ = {
@@ -115,10 +115,13 @@
{7947, "Thread-7"}, {7948, "FMOD mixer thre"},
{7950, "UnityGfxDeviceW"}, {7969, "UnityGfxDeviceW"},
};
+
+ Context context_;
+ TraceRedactor trace_redactor_;
};
TEST_F(RedactSchedSwitchIntegrationTest, ClearsNonTargetSwitchComms) {
- auto result = Redact();
+ auto result = Redact(trace_redactor_, &context_);
ASSERT_OK(result) << result.c_message();
auto original = LoadOriginal();
diff --git a/src/trace_redaction/scrub_process_stats_integrationtest.cc b/src/trace_redaction/scrub_process_stats_integrationtest.cc
index 148af4a..7f3d3d9 100644
--- a/src/trace_redaction/scrub_process_stats_integrationtest.cc
+++ b/src/trace_redaction/scrub_process_stats_integrationtest.cc
@@ -36,13 +36,13 @@
protected TraceRedactionIntegrationFixure {
protected:
void SetUp() override {
- trace_redactor()->emplace_collect<CollectTimelineEvents>();
+ trace_redactor_.emplace_collect<CollectTimelineEvents>();
- auto* scrub = trace_redactor()->emplace_transform<ScrubProcessStats>();
+ auto* scrub = trace_redactor_.emplace_transform<ScrubProcessStats>();
scrub->emplace_filter<ConnectedToPackage>();
// Package "com.Unity.com.unity.multiplayer.samples.coop";
- context()->package_uid = 10252;
+ context_.package_uid = 10252;
}
// Gets pids from all process_stats messages in the trace (bytes).
@@ -70,6 +70,9 @@
return pids;
}
+
+ Context context_;
+ TraceRedactor trace_redactor_;
};
// This test is a canary for changes to the test data. If the test data was to
@@ -128,7 +131,7 @@
// Package name: "com.Unity.com.unity.multiplayer.samples.coop"
// Package pid: 7105
TEST_F(ScrubProcessStatsTest, OnlyKeepsStatsForPackage) {
- auto result = Redact();
+ auto result = Redact(trace_redactor_, &context_);
ASSERT_OK(result) << result.c_message();
auto redacted = LoadRedacted();
diff --git a/src/trace_redaction/trace_redaction_integration_fixture.cc b/src/trace_redaction/trace_redaction_integration_fixture.cc
index 85555f3..9fc38d2 100644
--- a/src/trace_redaction/trace_redaction_integration_fixture.cc
+++ b/src/trace_redaction/trace_redaction_integration_fixture.cc
@@ -27,8 +27,10 @@
dest_trace_ = tmp_dir_.AbsolutePath("dst.pftrace");
}
-base::Status TraceRedactionIntegrationFixure::Redact() {
- auto status = trace_redactor_.Redact(src_trace_, dest_trace_, &context_);
+base::Status TraceRedactionIntegrationFixure::Redact(
+ const TraceRedactor& redactor,
+ Context* context) {
+ auto status = redactor.Redact(src_trace_, dest_trace_, context);
if (status.ok()) {
tmp_dir_.TrackFile("dst.pftrace");
diff --git a/src/trace_redaction/trace_redaction_integration_fixture.h b/src/trace_redaction/trace_redaction_integration_fixture.h
index 55060f9..2c695a2 100644
--- a/src/trace_redaction/trace_redaction_integration_fixture.h
+++ b/src/trace_redaction/trace_redaction_integration_fixture.h
@@ -32,23 +32,15 @@
// Redact the source file and write it to the destination file. The contents
// of each file can be read using LoadOriginal() and LoadRedacted().
- base::Status Redact();
+ base::Status Redact(const TraceRedactor& redactor, Context* context);
base::StatusOr<std::string> LoadOriginal() const;
base::StatusOr<std::string> LoadRedacted() const;
- Context* context() { return &context_; }
-
- TraceRedactor* trace_redactor() { return &trace_redactor_; }
-
private:
base::StatusOr<std::string> ReadRawTrace(const std::string& path) const;
- Context context_;
-
- TraceRedactor trace_redactor_;
-
base::TmpDirTree tmp_dir_;
std::string src_trace_;
