Skip to content

Commit 7a8c494

Browse files
jhassejhasse
authored
Merge pull request #2333 from digit-google/remove-stale-test-outputs
Ensure tests do not leave stale files in current directory.
2 parents b84b350 + eff5b44 commit 7a8c494

File tree

4 files changed

+113
-42
lines changed

4 files changed

+113
-42
lines changed

‎src/build_test.cc

Lines changed: 54 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -2228,8 +2228,8 @@ TEST_F(BuildTest, FailedDepsParse) {
22282228
}
22292229

22302230
struct BuildWithQueryDepsLogTest : public BuildTest {
2231-
BuildWithQueryDepsLogTest() : BuildTest(&log_) {
2232-
}
2231+
BuildWithQueryDepsLogTest()
2232+
: BuildTest(&log_), deps_log_file_("ninja_deps") {}
22332233

22342234
~BuildWithQueryDepsLogTest() {
22352235
log_.Close();
@@ -2241,12 +2241,13 @@ struct BuildWithQueryDepsLogTest : public BuildTest {
22412241
temp_dir_.CreateAndEnter("BuildWithQueryDepsLogTest");
22422242

22432243
std::string err;
2244-
ASSERT_TRUE(log_.OpenForWrite("ninja_deps", &err));
2244+
ASSERT_TRUE(log_.OpenForWrite(deps_log_file_.path(), &err));
22452245
ASSERT_EQ("", err);
22462246
}
22472247

22482248
ScopedTempDir temp_dir_;
22492249

2250+
ScopedFilePath deps_log_file_;
22502251
DepsLog log_;
22512252
};
22522253

@@ -2440,7 +2441,8 @@ TEST_F(BuildWithQueryDepsLogTest, TwoOutputsDepFileGCCOnlySecondaryOutput) {
24402441
/// builder_ it sets up, because we want pristine objects for
24412442
/// each build.
24422443
struct BuildWithDepsLogTest : public BuildTest {
2443-
BuildWithDepsLogTest() {}
2444+
BuildWithDepsLogTest()
2445+
: build_log_file_("build_log"), deps_log_file_("ninja_deps") {}
24442446

24452447
virtual void SetUp() {
24462448
BuildTest::SetUp();
@@ -2453,6 +2455,8 @@ struct BuildWithDepsLogTest : public BuildTest {
24532455
}
24542456

24552457
ScopedTempDir temp_dir_;
2458+
ScopedFilePath build_log_file_;
2459+
ScopedFilePath deps_log_file_;
24562460

24572461
/// Shadow parent class builder_ so we don't accidentally use it.
24582462
void* builder_;
@@ -2466,14 +2470,15 @@ TEST_F(BuildWithDepsLogTest, Straightforward) {
24662470
"build out: cat in1\n"
24672471
" deps = gcc\n"
24682472
" depfile = in1.d\n";
2473+
24692474
{
24702475
State state;
24712476
ASSERT_NO_FATAL_FAILURE(AddCatRule(&state));
24722477
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
24732478

24742479
// Run the build once, everything should be ok.
24752480
DepsLog deps_log;
2476-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
2481+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
24772482
ASSERT_EQ("", err);
24782483

24792484
Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);
@@ -2503,8 +2508,8 @@ TEST_F(BuildWithDepsLogTest, Straightforward) {
25032508

25042509
// Run the build again.
25052510
DepsLog deps_log;
2506-
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
2507-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
2511+
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
2512+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
25082513

25092514
Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);
25102515
builder.command_runner_.reset(&command_runner_);
@@ -2544,7 +2549,7 @@ TEST_F(BuildWithDepsLogTest, ObsoleteDeps) {
25442549

25452550
// Run the build once, everything should be ok.
25462551
DepsLog deps_log;
2547-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
2552+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
25482553
ASSERT_EQ("", err);
25492554

25502555
Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);
@@ -2573,8 +2578,8 @@ TEST_F(BuildWithDepsLogTest, ObsoleteDeps) {
25732578
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
25742579

25752580
DepsLog deps_log;
2576-
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
2577-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
2581+
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
2582+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
25782583

25792584
Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);
25802585
builder.command_runner_.reset(&command_runner_);
@@ -2638,12 +2643,12 @@ TEST_F(BuildWithDepsLogTest, TestInputMtimeRaceCondition) {
26382643
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
26392644

26402645
BuildLog build_log;
2641-
ASSERT_TRUE(build_log.Load("build_log", &err));
2642-
ASSERT_TRUE(build_log.OpenForWrite("build_log", *this, &err));
2646+
ASSERT_TRUE(build_log.Load(build_log_file_.path(), &err));
2647+
ASSERT_TRUE(build_log.OpenForWrite(build_log_file_.path(), *this, &err));
26432648

26442649
DepsLog deps_log;
2645-
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
2646-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
2650+
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
2651+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
26472652

26482653
BuildLog::LogEntry* log_entry = NULL;
26492654
{
@@ -2720,12 +2725,12 @@ TEST_F(BuildWithDepsLogTest, TestInputMtimeRaceConditionWithDepFile) {
27202725
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
27212726

27222727
BuildLog build_log;
2723-
ASSERT_TRUE(build_log.Load("build_log", &err));
2724-
ASSERT_TRUE(build_log.OpenForWrite("build_log", *this, &err));
2728+
ASSERT_TRUE(build_log.Load(build_log_file_.path(), &err));
2729+
ASSERT_TRUE(build_log.OpenForWrite(build_log_file_.path(), *this, &err));
27252730

27262731
DepsLog deps_log;
2727-
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
2728-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
2732+
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
2733+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
27292734

27302735
{
27312736
Builder builder(&state, config_, &build_log, &deps_log, &fs_, &status_, 0);
@@ -2871,7 +2876,7 @@ TEST_F(BuildWithDepsLogTest, RestatDepfileDependencyDepsLog) {
28712876

28722877
// Run the build once, everything should be ok.
28732878
DepsLog deps_log;
2874-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
2879+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
28752880
ASSERT_EQ("", err);
28762881

28772882
Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);
@@ -2897,8 +2902,8 @@ TEST_F(BuildWithDepsLogTest, RestatDepfileDependencyDepsLog) {
28972902

28982903
// Run the build again.
28992904
DepsLog deps_log;
2900-
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
2901-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
2905+
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
2906+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
29022907

29032908
Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);
29042909
builder.command_runner_.reset(&command_runner_);
@@ -2930,7 +2935,7 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) {
29302935

29312936
// Run the build once, everything should be ok.
29322937
DepsLog deps_log;
2933-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
2938+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
29342939
ASSERT_EQ("", err);
29352940

29362941
Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);
@@ -2950,8 +2955,8 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) {
29502955
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
29512956

29522957
DepsLog deps_log;
2953-
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
2954-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
2958+
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
2959+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
29552960
ASSERT_EQ("", err);
29562961

29572962
Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);
@@ -3001,7 +3006,7 @@ TEST_F(BuildWithDepsLogTest, DiscoveredDepDuringBuildChanged) {
30013006
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
30023007

30033008
DepsLog deps_log;
3004-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
3009+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
30053010
ASSERT_EQ("", err);
30063011

30073012
Builder builder(&state, config_, &build_log, &deps_log, &fs_, &status_, 0);
@@ -3024,8 +3029,8 @@ TEST_F(BuildWithDepsLogTest, DiscoveredDepDuringBuildChanged) {
30243029
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
30253030

30263031
DepsLog deps_log;
3027-
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
3028-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
3032+
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
3033+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
30293034
ASSERT_EQ("", err);
30303035

30313036
Builder builder(&state, config_, &build_log, &deps_log, &fs_, &status_, 0);
@@ -3047,8 +3052,8 @@ TEST_F(BuildWithDepsLogTest, DiscoveredDepDuringBuildChanged) {
30473052
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
30483053

30493054
DepsLog deps_log;
3050-
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
3051-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
3055+
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
3056+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
30523057
ASSERT_EQ("", err);
30533058

30543059
Builder builder(&state, config_, &build_log, &deps_log, &fs_, &status_, 0);
@@ -3076,7 +3081,7 @@ TEST_F(BuildWithDepsLogTest, DepFileDepsLogCanonicalize) {
30763081

30773082
// Run the build once, everything should be ok.
30783083
DepsLog deps_log;
3079-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
3084+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
30803085
ASSERT_EQ("", err);
30813086

30823087
Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);
@@ -3098,8 +3103,8 @@ TEST_F(BuildWithDepsLogTest, DepFileDepsLogCanonicalize) {
30983103
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
30993104

31003105
DepsLog deps_log;
3101-
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
3102-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
3106+
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
3107+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
31033108
ASSERT_EQ("", err);
31043109

31053110
Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);
@@ -3169,11 +3174,13 @@ TEST_F(BuildWithDepsLogTest, RestatMissingDepfileDepslog) {
31693174
fs_.Create("out.d", "out: header.h");
31703175
fs_.Create("header.h", "");
31713176

3172-
RebuildTarget("out", manifest, "build_log", "ninja_deps");
3177+
RebuildTarget("out", manifest, build_log_file_.c_str(),
3178+
deps_log_file_.c_str());
31733179
ASSERT_EQ(2u, command_runner_.commands_ran_.size());
31743180

31753181
// Sanity: this rebuild should be NOOP
3176-
RebuildTarget("out", manifest, "build_log", "ninja_deps");
3182+
RebuildTarget("out", manifest, build_log_file_.c_str(),
3183+
deps_log_file_.c_str());
31773184
ASSERT_EQ(0u, command_runner_.commands_ran_.size());
31783185

31793186
// Touch 'header.in', blank dependencies log (create a different one).
@@ -3182,24 +3189,26 @@ TEST_F(BuildWithDepsLogTest, RestatMissingDepfileDepslog) {
31823189
fs_.Tick();
31833190
fs_.Create("header.in", "");
31843191

3192+
ScopedFilePath deps2_file_("ninja_deps2");
3193+
31853194
// (switch to a new blank deps_log "ninja_deps2")
3186-
RebuildTarget("out", manifest, "build_log", "ninja_deps2");
3195+
RebuildTarget("out", manifest, build_log_file_.c_str(), deps2_file_.c_str());
31873196
ASSERT_EQ(2u, command_runner_.commands_ran_.size());
31883197

31893198
// Sanity: this build should be NOOP
3190-
RebuildTarget("out", manifest, "build_log", "ninja_deps2");
3199+
RebuildTarget("out", manifest, build_log_file_.c_str(), deps2_file_.c_str());
31913200
ASSERT_EQ(0u, command_runner_.commands_ran_.size());
31923201

31933202
// Check that invalidating deps by target timestamp also works here
31943203
// Repeat the test but touch target instead of blanking the log.
31953204
fs_.Tick();
31963205
fs_.Create("header.in", "");
31973206
fs_.Create("out", "");
3198-
RebuildTarget("out", manifest, "build_log", "ninja_deps2");
3207+
RebuildTarget("out", manifest, build_log_file_.c_str(), deps2_file_.c_str());
31993208
ASSERT_EQ(2u, command_runner_.commands_ran_.size());
32003209

32013210
// And this build should be NOOP again
3202-
RebuildTarget("out", manifest, "build_log", "ninja_deps2");
3211+
RebuildTarget("out", manifest, build_log_file_.c_str(), deps2_file_.c_str());
32033212
ASSERT_EQ(0u, command_runner_.commands_ran_.size());
32043213
}
32053214

@@ -3216,7 +3225,10 @@ TEST_F(BuildTest, WrongOutputInDepfileCausesRebuild) {
32163225
fs_.Create("header.h", "");
32173226
fs_.Create("foo.o.d", "bar.o.d: header.h\n");
32183227

3219-
RebuildTarget("foo.o", manifest, "build_log", "ninja_deps");
3228+
ScopedFilePath build_log("build_log");
3229+
ScopedFilePath deps_file("ninja_deps");
3230+
3231+
RebuildTarget("foo.o", manifest, build_log.c_str(), deps_file.c_str());
32203232
ASSERT_EQ(1u, command_runner_.commands_ran_.size());
32213233
}
32223234

@@ -4173,7 +4185,7 @@ TEST_F(BuildWithDepsLogTest, ValidationThroughDepfile) {
41734185
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
41744186

41754187
DepsLog deps_log;
4176-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
4188+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
41774189
ASSERT_EQ("", err);
41784190

41794191
Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);
@@ -4208,8 +4220,8 @@ TEST_F(BuildWithDepsLogTest, ValidationThroughDepfile) {
42084220
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
42094221

42104222
DepsLog deps_log;
4211-
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
4212-
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
4223+
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
4224+
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
42134225
ASSERT_EQ("", err);
42144226

42154227
Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);

‎src/missing_deps_test.cc

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,11 @@ struct MissingDependencyScannerTest : public testing::Test {
3636
ASSERT_EQ("", err);
3737
}
3838

39+
~MissingDependencyScannerTest() {
40+
// Remove test file.
41+
deps_log_.Close();
42+
}
43+
3944
MissingDependencyScanner& scanner() { return scanner_; }
4045

4146
void RecordDepsLogDep(const std::string& from, const std::string& to) {
@@ -79,6 +84,7 @@ struct MissingDependencyScannerTest : public testing::Test {
7984
ASSERT_EQ(1u, scanner().generator_rules_.count(rule));
8085
}
8186

87+
ScopedFilePath scoped_file_path_ = kTestDepsLogFilename;
8288
MissingDependencyTestDelegate delegate_;
8389
Rule generator_rule_;
8490
Rule compile_rule_;

‎src/test.cc

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,3 +235,29 @@ void ScopedTempDir::Cleanup() {
235235

236236
temp_dir_name_.clear();
237237
}
238+
239+
ScopedFilePath::ScopedFilePath(ScopedFilePath&& other) noexcept
240+
: path_(std::move(other.path_)), released_(other.released_) {
241+
other.released_ = true;
242+
}
243+
244+
/// It would be nice to use '= default' here instead but some old compilers
245+
/// such as GCC from Ubuntu 16.06 will not compile it with "noexcept", so just
246+
/// write it manually.
247+
ScopedFilePath& ScopedFilePath::operator=(ScopedFilePath&& other) noexcept {
248+
if (this != &other) {
249+
this->~ScopedFilePath();
250+
new (this) ScopedFilePath(std::move(other));
251+
}
252+
return *this;
253+
}
254+
255+
ScopedFilePath::~ScopedFilePath() {
256+
if (!released_) {
257+
unlink(path_.c_str());
258+
}
259+
}
260+
261+
void ScopedFilePath::Release() {
262+
released_ = true;
263+
}

‎src/test.h

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,4 +182,31 @@ struct ScopedTempDir {
182182
std::string temp_dir_name_;
183183
};
184184

185+
/// A class that records a file path and ensures that it is removed
186+
/// on destruction. This ensures that tests do not keep stale files in the
187+
/// current directory where they run, even in case of assertion failure.
188+
struct ScopedFilePath {
189+
/// Constructor just records the file path.
190+
ScopedFilePath(const std::string& path) : path_(path) {}
191+
ScopedFilePath(const char* path) : path_(path) {}
192+
193+
/// Allow move operations.
194+
ScopedFilePath(ScopedFilePath&&) noexcept;
195+
ScopedFilePath& operator=(ScopedFilePath&&) noexcept;
196+
197+
/// Destructor destroys the file, unless Release() was called.
198+
~ScopedFilePath();
199+
200+
/// Release the file, the destructor will not remove the file.
201+
void Release();
202+
203+
const char* c_str() const { return path_.c_str(); }
204+
const std::string& path() const { return path_; }
205+
bool released() const { return released_; }
206+
207+
private:
208+
std::string path_;
209+
bool released_ = false;
210+
};
211+
185212
#endif // NINJA_TEST_H_

0 commit comments

Comments
 (0)