Skip to content

Commit

Permalink
Ensure tests do not leave stale files in current directory.
Browse files Browse the repository at this point in the history 
This patch fixes a minor but annoying issue during development
where some tests would leave stale files in the current
directory.

+ Introduce new ScopedFilePath class to perform
  remove-on-scope-exit of a given file path.

Fixes #1583
  • Loading branch information
@digit-android @digit-google
digit-android authored and digit-google committed Oct 6, 2023
1 parent 93d0d35 commit eff5b44
Show file tree
Hide file tree
Showing 4 changed files with 113 additions and 42 deletions.
96 changes: 54 additions & 42 deletions src/build_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2228,8 +2228,8 @@ TEST_F(BuildTest, FailedDepsParse) {
}

struct BuildWithQueryDepsLogTest : public BuildTest {
BuildWithQueryDepsLogTest() : BuildTest(&log_) {
}
BuildWithQueryDepsLogTest()
: BuildTest(&log_), deps_log_file_("ninja_deps") {}

~BuildWithQueryDepsLogTest() {
log_.Close();
Expand All @@ -2241,12 +2241,13 @@ struct BuildWithQueryDepsLogTest : public BuildTest {
temp_dir_.CreateAndEnter("BuildWithQueryDepsLogTest");

std::string err;
ASSERT_TRUE(log_.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(log_.OpenForWrite(deps_log_file_.path(), &err));
ASSERT_EQ("", err);
}

ScopedTempDir temp_dir_;

ScopedFilePath deps_log_file_;
DepsLog log_;
};

Expand Down Expand Up @@ -2440,7 +2441,8 @@ TEST_F(BuildWithQueryDepsLogTest, TwoOutputsDepFileGCCOnlySecondaryOutput) {
/// builder_ it sets up, because we want pristine objects for
/// each build.
struct BuildWithDepsLogTest : public BuildTest {
BuildWithDepsLogTest() {}
BuildWithDepsLogTest()
: build_log_file_("build_log"), deps_log_file_("ninja_deps") {}

virtual void SetUp() {
BuildTest::SetUp();
Expand All @@ -2453,6 +2455,8 @@ struct BuildWithDepsLogTest : public BuildTest {
}

ScopedTempDir temp_dir_;
ScopedFilePath build_log_file_;
ScopedFilePath deps_log_file_;

/// Shadow parent class builder_ so we don't accidentally use it.
void* builder_;
Expand All @@ -2466,14 +2470,15 @@ TEST_F(BuildWithDepsLogTest, Straightforward) {
"build out: cat in1\n"
" deps = gcc\n"
" depfile = in1.d\n";

{
State state;
ASSERT_NO_FATAL_FAILURE(AddCatRule(&state));
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));

// Run the build once, everything should be ok.
DepsLog deps_log;
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
ASSERT_EQ("", err);

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

// Run the build again.
DepsLog deps_log;
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));

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

// Run the build once, everything should be ok.
DepsLog deps_log;
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
ASSERT_EQ("", err);

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

DepsLog deps_log;
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));

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

BuildLog build_log;
ASSERT_TRUE(build_log.Load("build_log", &err));
ASSERT_TRUE(build_log.OpenForWrite("build_log", *this, &err));
ASSERT_TRUE(build_log.Load(build_log_file_.path(), &err));
ASSERT_TRUE(build_log.OpenForWrite(build_log_file_.path(), *this, &err));

DepsLog deps_log;
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));

BuildLog::LogEntry* log_entry = NULL;
{
Expand Down Expand Up @@ -2720,12 +2725,12 @@ TEST_F(BuildWithDepsLogTest, TestInputMtimeRaceConditionWithDepFile) {
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));

BuildLog build_log;
ASSERT_TRUE(build_log.Load("build_log", &err));
ASSERT_TRUE(build_log.OpenForWrite("build_log", *this, &err));
ASSERT_TRUE(build_log.Load(build_log_file_.path(), &err));
ASSERT_TRUE(build_log.OpenForWrite(build_log_file_.path(), *this, &err));

DepsLog deps_log;
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));

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

// Run the build once, everything should be ok.
DepsLog deps_log;
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
ASSERT_EQ("", err);

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

// Run the build again.
DepsLog deps_log;
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));

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

// Run the build once, everything should be ok.
DepsLog deps_log;
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
ASSERT_EQ("", err);

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

DepsLog deps_log;
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
ASSERT_EQ("", err);

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

DepsLog deps_log;
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
ASSERT_EQ("", err);

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

DepsLog deps_log;
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
ASSERT_EQ("", err);

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

DepsLog deps_log;
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
ASSERT_EQ("", err);

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

// Run the build once, everything should be ok.
DepsLog deps_log;
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
ASSERT_EQ("", err);

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

DepsLog deps_log;
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
ASSERT_EQ("", err);

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

RebuildTarget("out", manifest, "build_log", "ninja_deps");
RebuildTarget("out", manifest, build_log_file_.c_str(),
deps_log_file_.c_str());
ASSERT_EQ(2u, command_runner_.commands_ran_.size());

// Sanity: this rebuild should be NOOP
RebuildTarget("out", manifest, "build_log", "ninja_deps");
RebuildTarget("out", manifest, build_log_file_.c_str(),
deps_log_file_.c_str());
ASSERT_EQ(0u, command_runner_.commands_ran_.size());

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

ScopedFilePath deps2_file_("ninja_deps2");

// (switch to a new blank deps_log "ninja_deps2")
RebuildTarget("out", manifest, "build_log", "ninja_deps2");
RebuildTarget("out", manifest, build_log_file_.c_str(), deps2_file_.c_str());
ASSERT_EQ(2u, command_runner_.commands_ran_.size());

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

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

// And this build should be NOOP again
RebuildTarget("out", manifest, "build_log", "ninja_deps2");
RebuildTarget("out", manifest, build_log_file_.c_str(), deps2_file_.c_str());
ASSERT_EQ(0u, command_runner_.commands_ran_.size());
}

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

RebuildTarget("foo.o", manifest, "build_log", "ninja_deps");
ScopedFilePath build_log("build_log");
ScopedFilePath deps_file("ninja_deps");

RebuildTarget("foo.o", manifest, build_log.c_str(), deps_file.c_str());
ASSERT_EQ(1u, command_runner_.commands_ran_.size());
}

Expand Down Expand Up @@ -4173,7 +4185,7 @@ TEST_F(BuildWithDepsLogTest, ValidationThroughDepfile) {
ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));

DepsLog deps_log;
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
ASSERT_EQ("", err);

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

DepsLog deps_log;
ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
ASSERT_TRUE(deps_log.Load(deps_log_file_.path(), &state, &err));
ASSERT_TRUE(deps_log.OpenForWrite(deps_log_file_.path(), &err));
ASSERT_EQ("", err);

Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);
Expand Down
6 changes: 6 additions & 0 deletions src/missing_deps_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ struct MissingDependencyScannerTest : public testing::Test {
ASSERT_EQ("", err);
}

~MissingDependencyScannerTest() {
// Remove test file.
deps_log_.Close();
}

MissingDependencyScanner& scanner() { return scanner_; }

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

ScopedFilePath scoped_file_path_ = kTestDepsLogFilename;
MissingDependencyTestDelegate delegate_;
Rule generator_rule_;
Rule compile_rule_;
Expand Down
26 changes: 26 additions & 0 deletions src/test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,3 +235,29 @@ void ScopedTempDir::Cleanup() {

temp_dir_name_.clear();
}

ScopedFilePath::ScopedFilePath(ScopedFilePath&& other) noexcept
: path_(std::move(other.path_)), released_(other.released_) {
other.released_ = true;
}

/// It would be nice to use '= default' here instead but some old compilers
/// such as GCC from Ubuntu 16.06 will not compile it with "noexcept", so just
/// write it manually.
ScopedFilePath& ScopedFilePath::operator=(ScopedFilePath&& other) noexcept {
if (this != &other) {
this->~ScopedFilePath();
new (this) ScopedFilePath(std::move(other));
}
return *this;
}

ScopedFilePath::~ScopedFilePath() {
if (!released_) {
unlink(path_.c_str());
}
}

void ScopedFilePath::Release() {
released_ = true;
}
27 changes: 27 additions & 0 deletions src/test.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,4 +182,31 @@ struct ScopedTempDir {
std::string temp_dir_name_;
};

/// A class that records a file path and ensures that it is removed
/// on destruction. This ensures that tests do not keep stale files in the
/// current directory where they run, even in case of assertion failure.
struct ScopedFilePath {
/// Constructor just records the file path.
ScopedFilePath(const std::string& path) : path_(path) {}
ScopedFilePath(const char* path) : path_(path) {}

/// Allow move operations.
ScopedFilePath(ScopedFilePath&&) noexcept;
ScopedFilePath& operator=(ScopedFilePath&&) noexcept;

/// Destructor destroys the file, unless Release() was called.
~ScopedFilePath();

/// Release the file, the destructor will not remove the file.
void Release();

const char* c_str() const { return path_.c_str(); }
const std::string& path() const { return path_; }
bool released() const { return released_; }

private:
std::string path_;
bool released_ = false;
};

#endif // NINJA_TEST_H_

0 comments on commit eff5b44

Please sign in to comment.