aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorBoris Kolpackov <boris@codesynthesis.com>2016-11-04 08:51:53 +0200
committerBoris Kolpackov <boris@codesynthesis.com>2016-11-04 09:26:34 +0200
commit8e3a8ffa6579a51f5a9351e1b99c07d3e1fbd234 (patch)
tree11083ee4ec31aacce6b071440d465f66cee6dd7f
parentebe762d3d6300dc9a5177b800876d5a65dbe996e (diff)
Rework test runner
-rw-r--r--build2/test/script/runner4
-rw-r--r--build2/test/script/runner.cxx413
-rw-r--r--build2/test/script/script2
-rw-r--r--build2/utility.cxx3
4 files changed, 202 insertions, 220 deletions
diff --git a/build2/test/script/runner b/build2/test/script/runner
index c4f87ee..0180108 100644
--- a/build2/test/script/runner
+++ b/build2/test/script/runner
@@ -47,13 +47,13 @@ namespace build2
{
public:
virtual void
- enter (scope&, const location&) override {}
+ enter (scope&, const location&) override;
virtual void
run (scope&, const command&, size_t, const location&) override;
virtual void
- leave (scope&, const location&) override {}
+ leave (scope&, const location&) override;
};
}
}
diff --git a/build2/test/script/runner.cxx b/build2/test/script/runner.cxx
index a4fb31e..510b61d 100644
--- a/build2/test/script/runner.cxx
+++ b/build2/test/script/runner.cxx
@@ -2,14 +2,10 @@
// copyright : Copyright (c) 2014-2016 Code Synthesis Ltd
// license : MIT; see accompanying LICENSE file
-#ifndef _WIN32
-# include <sys/wait.h> // WIFEXITED(), WEXITSTATUS()
-#endif
-
+#include <set>
#include <iostream> // cerr
-#include <butl/filesystem> // auto_rm
-
+#include <build2/filesystem>
#include <build2/test/script/runner>
using namespace std;
@@ -21,141 +17,105 @@ namespace build2
{
namespace script
{
- // Test command output cache. The usage is as follows:
- //
- // 1. Call wopen() to open the stream in write mode and register the file
- // for auto-removal by dtor.
- //
- // 2. Pass the file descriptor to the test command process ctor to
- // redirect it's output to the cache file.
+ // @@ Eventually the cleanup facility should move into the scope object.
//
- // 3. Close the stream.
- //
- // 4. Call ropen() to open the file in read mode to match the content to
- // a pattern, cancel the file removal if the match fails (so the
- // output is available for troubleshooting).
- //
- class stream_cache: public ifdstream // @@ Open for output?
+ class cleanup
{
public:
- using path_type = butl::path;
+ void
+ add (path p) {files_.emplace (move (p));}
- stream_cache (const char* n): name (n) {}
+ void
+ cancel (const path& p) {files_.erase (p);}
- // Open stream for writing. Return the file descriptor. Must not be
- // called multiple times.
- //
- int
- wopen ()
+ ~cleanup ()
{
- // Otherwise compiler gets confused with basic_ios::fail().
- //
- using build2::fail;
-
- assert (!exists ());
-
- try
- {
- // @@ Later it will make sense to create the file in the
- // test-specific temporary directory.
- //
- path = path_type::temp_path ("build2");
- }
- catch (const system_error& e)
- {
- fail << "unable to create temporary file: " << e.what ();
- }
-
- try
- {
- open (path, out | trunc);
- cleanup = auto_rm<path_type> (path);
- }
- catch (const io_error& e)
- {
- fail << "unable to write " << path << ": " << e.what ();
- }
-
- return fd ();
+ for (const auto& p: files_)
+ try_rmfile (p, true);
}
- // Open stream for reading. Return true if the file is not empty,
- // false otherwise. Must not be called before wopen().
- //
- bool
- ropen ()
- {
- // Otherwise compiler gets confused with basic_ios::fail().
- //
- using build2::fail;
+ private:
+ set<path> files_;
+ };
- assert (exists ());
+ // Check if the file exists and is not empty.
+ //
+ static bool
+ non_empty (const path& p)
+ {
+ if (!exists (p))
+ return false;
- try
- {
- open (path, in);
- return peek () != ifdstream::traits_type::eof ();
- }
- catch (const io_error& e)
- {
- error << "unable to read " << path << ": " << e.what ();
- throw failed ();
- }
+ try
+ {
+ ifdstream is (p);
+ return is.peek () != ifdstream::traits_type::eof ();
}
-
- // Return true if wopen() was called, false otherwise.
- //
- bool
- exists () const {return !path.empty ();}
-
- ~stream_cache () override
+ catch (const io_error& e)
{
- close (); // Close the stream prior to the file deletion.
+ error << "unable to read " << p << ": " << e.what ();
+ throw failed ();
}
+ }
- public:
- string name;
- path_type path;
- auto_rm<path_type> cleanup;
- };
-
- // Check if the test command output matches the pattern (redirect value).
- //
- // @@ Expected result | expected output
+ // Check if the test command output matches the expected result (redirect
+ // value).
//
static void
- check_output (const process_path& program,
- stream_cache& sc,
- const redirect& rd)
+ check_output (const process_path& pr,
+ const char* nm,
+ const path& op,
+ const redirect& rd,
+ const location& cl,
+ cleanup& cln)
{
if (rd.type == redirect_type::none)
{
- // Check that the cache file is empty.
+ // Check that there is no output produced.
//
- if (sc.ropen ())
+ if (non_empty (op))
{
- sc.cleanup.cancel ();
+ cln.cancel (op);
- fail << program << " unexpectedly writes to " << sc.name <<
- info << sc.name << " is saved to " << sc.path;
+ fail (cl) << pr << " unexpectedly writes to " << nm <<
+ info << nm << " is saved to " << op;
}
}
else if (rd.type == redirect_type::here_string ||
rd.type == redirect_type::here_document)
{
- // Use diff utility to compare the output with the pattern.
+ path orp (op + ".orig");
+
+ try
+ {
+ ofdstream os (orp);
+ cln.add (orp);
+
+ os << rd.value;
+
+ // Here-document is always newline-terminated.
+ //
+ if (rd.type == redirect_type::here_string)
+ os << endl;
+
+ os.close ();
+ }
+ catch (const io_error& e)
+ {
+ fail << "unable to write " << orp << ": " << e.what ();
+ }
+
+ // Use diff utility to compare the output with the expected result.
//
path dp ("diff");
process_path pp (run_search (dp, true));
- //@@ Need to compare both as files.
-
cstrings args {
pp.recall_string (),
"--strip-trailing-cr",
"-u",
- "-",
- sc.path.string ().c_str (),
+ orp.string ().c_str (),
+ op.string ().c_str (),
nullptr};
if (verb >= 2)
@@ -167,71 +127,45 @@ namespace build2
// it is a part of the test failure diagnostics so let's redirect
// stdout to stderr.
//
- process pr (pp, args.data (), -1, 2);
+ process p (pp, args.data (), 0, 2);
try
{
- ofdstream os (pr.out_fd);
-
- auto write_value = [&os, &rd] ()
- {
- os << rd.value;
-
- // Here-document is always endline-terminated.
- //
- // @@ newline
- //
- if (rd.type == redirect_type::here_string)
- os << endl;
-
- os.close ();
- };
-
- write_value ();
-
- if (pr.wait ())
+ if (p.wait ())
return;
- // Output doesn't match the pattern string. Keep non-empty output
- // and save the pattern for troubleshooting.
+ // Output doesn't match the expected result. Keep non-empty
+ // output and the expected result for troubleshooting.
//
- path p (sc.path + ".pattern"); // @@ .orig
+ cln.cancel (orp);
- try
- {
- os.open (p);
- write_value ();
- }
- catch (const io_error& e)
- {
- fail << "unable to write " << p << ": " << e.what ();
- }
+ diag_record d (error (cl));
+ d << pr << " " << nm << " doesn't match the expected output";
- diag_record d (error);
- d << program << " " << sc.name
- << " doesn't match the pattern";
-
- if (sc.ropen ())
+ if (non_empty (op))
{
- sc.cleanup.cancel ();
- d << info << sc.name << " is saved to " << sc.path;
+ cln.cancel (op);
+ d << info << nm << " is saved to " << op;
}
else
- d << info << sc.name << " is empty";
+ d << info << nm << " is empty";
- // Pattern is never empty (contains at least newline).
+ // Expected output is never empty (contains at least newline).
//
- d << info << sc.name << " pattern is saved to " << p;
+ d << info << "expected " << nm << " is saved to " << orp;
// Fall through.
//
}
- catch (const io_error& e)
+ catch (const io_error&)
{
// Child exit status doesn't matter. Assume the child process
// issued diagnostics. Just wait for the process completion.
//
- pr.wait (); // Check throw.
+ p.wait (); // Check throw.
+
+ error (cl) << "failed to compare " << nm
+ << " with the expected output";
}
// Fall through.
@@ -239,7 +173,7 @@ namespace build2
}
catch (const process_error& e)
{
- error << "unable to execute " << args[0] << ": " << e.what ();
+ error (cl) << "unable to execute " << pp << ": " << e.what ();
if (e.child ())
exit (1);
@@ -250,7 +184,26 @@ namespace build2
}
void concurrent_runner::
- run (scope&, const command& c, size_t ci, const location& cl)
+ enter (scope& sp, const location&)
+ {
+ if (!exists (sp.wd_path))
+ mkdir (sp.wd_path, 2);
+ else if (!empty (sp.wd_path))
+ // @@ Shouldn't we have --wipe or smth?
+ //
+ fail << "directory " << sp.wd_path << " is not empty" <<
+ info << "clean it up and rerun";
+ }
+
+ void concurrent_runner::
+ leave (scope& sp, const location&)
+ {
+ if (exists (sp.wd_path) && empty (sp.wd_path))
+ rmdir (sp.wd_path, 2);
+ }
+
+ void concurrent_runner::
+ run (scope& sp, const command& c, size_t ci, const location& cl)
{
if (verb >= 3)
text << c;
@@ -267,15 +220,6 @@ namespace build2
args.push_back (nullptr);
- // Normally while handling child process failures (IO errors, non-zero
- // exit status) we suppress the diagnostics supposing that the child
- // issues it's own one. While this is reasonable to expect from known
- // production-quality programs here it can result in the absense of any
- // diagnostics at all. Also the child stderr (and so diagnostics) can
- // be redirected to /dev/null and not be available for the user. This
- // why we will always issue the diagnostics despite the fact sometimes
- // it can look redundant.
- //
try
{
// For stdin 'none' redirect type we somehow need to make sure that
@@ -300,31 +244,69 @@ namespace build2
// using pipes is tricky in the general case. Going this path we
// would need to read both streams in a non-blocking manner which we
// can't (easily) do in a portable way. Using diff utility to get a
- // nice-looking stream/pattern difference would complicate things
- // further.
+ // nice-looking actual/expected output difference would complicate
+ // things further.
//
// So the approach is the following. Child standard stream are
// redirected to files. When the child exits and the exit status is
// validated we just sequentially compare each file content with the
- // corresponding pattern. The positive side-effect of this approach
- // is that the output of a faulty test command can be provided for
+ // expected output. The positive side-effect of this approach is that
+ // the output of a faulty test command can be provided for
// troubleshooting.
//
- stream_cache osc ("stdout");
- int out (c.out.type == redirect_type::null ? -2 : osc.wopen ());
+ cleanup cln;
+
+ auto opath = [sp, ci] (const char* nm) -> path
+ {
+ path r (sp.wd_path / path (nm));
- stream_cache esc ("stderr");
- int err (c.err.type == redirect_type::null ? -2 : esc.wopen ());
+ if (ci > 0)
+ r += "-" + to_string (ci);
+
+ return r;
+ };
+
+ auto open = [&cln] (ofdstream& os, const path& p) -> int
+ {
+ try
+ {
+ os.open (p);
+ cln.add (p);
+ }
+ catch (const io_error& e)
+ {
+ fail << "unable to write " << p << ": " << e.what ();
+ }
+
+ return os.fd ();
+ };
+
+ ofdstream so;
+ path stdout (opath ("stdout"));
+
+ int out (c.out.type == redirect_type::null
+ ? -2
+ : open (so, stdout));
+
+ ofdstream se;
+ path stderr (opath ("stderr"));
+
+ int err (c.err.type == redirect_type::null
+ ? -2
+ : open (se, stderr));
if (verb >= 2)
print_process (args);
- process pr (pp, args.data (), in, out, err);
+ process pr (sp.wd_path.string ().c_str (),
+ pp,
+ args.data (),
+ in, out, err);
try
{
- osc.close ();
- esc.close ();
+ so.close ();
+ se.close ();
if (c.in.type == redirect_type::here_string ||
c.in.type == redirect_type::here_document)
@@ -332,7 +314,7 @@ namespace build2
ofdstream os (pr.out_fd);
os << c.in.value;
- // Here-document is always endline-terminated.
+ // Here-document is always newline-terminated.
//
if (c.in.type == redirect_type::here_string)
os << endl;
@@ -344,74 +326,69 @@ namespace build2
//
pr.wait ();
- // Check if the process terminated normally and obtain the status
- // if that's the case.
- //
- bool abnorm;
- process::status_type status;
-
- // @@ Shouldn't we incorporate means for checking for abnormal
- // termination and getting the real exit status into
- // libbutl::process?
- //
- // Yes, sounds good.
- //
-#ifndef _WIN32
- abnorm = !WIFEXITED (pr.status);
- status = abnorm ? 1 : WEXITSTATUS (pr.status);
-#else
- // @@ Is there a reliable way to detect if the process terminated
- // abnormally on Windows?
+ // If there is no correct exit status by whatever reason then dump
+ // stderr (if cached), keep both stdout and stderr (those which
+ // are cached) for troubleshooting, print the proper diagnostics
+ // and finally fail.
//
- abnorm = false;
- status = pr.status;
-#endif
- bool valid_status (!abnorm && status >= 0 && status < 256);
-
+ optional<process::status_type> status (move (pr.status));
+ bool valid_status (status && *status >= 0 && *status < 256);
bool eq (c.exit.comparison == exit_comparison::eq);
bool correct_status (valid_status &&
- (status == c.exit.status) == eq);
+ (*status == c.exit.status) == eq);
- // If there is no correct exit status by whatever reason then dump
- // stderr (if cached), keep both stdout and stderr (those which
- // are cached) for troubleshooting, and finally fail.
- //
if (!correct_status)
{
- if (esc.exists () && esc.ropen ())
- cerr << esc.rdbuf ();
+ // Dump cached stderr.
+ //
+ if (exists (stderr))
+ {
+ try
+ {
+ ifdstream is (stderr);
+ if (is.peek () != ifdstream::traits_type::eof ())
+ cerr << is.rdbuf ();
+ }
+ catch (const io_error& e)
+ {
+ fail << "unable to read " << stderr << ": " << e.what ();
+ }
+ }
// Keep non-empty cache files and fail with a proper diagnostics.
//
- diag_record d (fail);
+ diag_record d (fail (cl));
- if (abnorm)
+ if (!status)
d << pp << " terminated abnormally";
else if (!valid_status)
- d << pp << " exit status " << status << " is invalid" <<
+ d << pp << " exit status " << *status << " is invalid" <<
info << "must be an unsigned integer < 256";
else if (!correct_status)
- d << pp << " exit status " << status
- << (eq ? " != " : " == ") << (int)c.exit.status; //@@
+ d << pp << " exit status " << *status
+ << (eq ? " != " : " == ")
+ << static_cast<uint16_t> (c.exit.status);
else
assert (false);
- auto keep_stream = [&d] (stream_cache& sc)
+ auto keep_output = [&d, &cln] (const char* name, const path& p)
{
- if (sc.exists () && sc.ropen ())
+ if (non_empty (p))
{
- sc.cleanup.cancel ();
- d << info << sc.name << " is saved to " << sc.path;
+ cln.cancel (p);
+ d << info << name << " is saved to " << p;
}
};
- keep_stream (osc);
- keep_stream (esc);
+ keep_output ("stdout", stdout);
+ keep_output ("stderr", stderr);
}
- check_output (pp, osc, c.out);
- check_output (pp, esc, c.err);
+ // Check if the standard outputs match expectations.
+ //
+ check_output (pp, "stdout", stdout, c.out, cl, cln);
+ check_output (pp, "stderr", stderr, c.err, cl, cln);
}
catch (const io_error& e)
{
@@ -420,12 +397,12 @@ namespace build2
//
pr.wait (); // Check throw.
- fail << "IO operation failed for " << pp << ": " << e.what ();
+ fail (cl) << "IO operation failed for " << pp << ": " << e.what ();
}
}
catch (const process_error& e)
{
- error << "unable to execute " << pp << ": " << e.what ();
+ error (cl) << "unable to execute " << pp << ": " << e.what ();
if (e.child ())
exit (1);
diff --git a/build2/test/script/script b/build2/test/script/script
index 3095ccf..ff9fa46 100644
--- a/build2/test/script/script
+++ b/build2/test/script/script
@@ -22,6 +22,8 @@ namespace build2
{
namespace script
{
+ class parser; // Required by VC for 'friend class parser' declaration.
+
// Pre-parse representation.
//
enum class line_type {variable, test};
diff --git a/build2/utility.cxx b/build2/utility.cxx
index 0e97020..ca6b239 100644
--- a/build2/utility.cxx
+++ b/build2/utility.cxx
@@ -36,6 +36,9 @@ namespace build2
os << "<empty>";
else
{
+ // @@ Is there a reason not to print as a relative path as it is done
+ // for path (see above)?
+ //
os << p.recall_string ();
if (!p.effect.empty ())