[libcamera-devel] [PATCH 3/4] test: logging: add logging process test

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sat, Jul 13, 2019 at 05:16:19AM +0900, Paul Elder wrote:
> Add a test to test that logging works in child processes. Only
> logSetFile is tested.

Can you explain why ?

> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  test/log_process.cpp | 147 +++++++++++++++++++++++++++++++++++++++++++
>  test/meson.build     |   1 +
>  2 files changed, 148 insertions(+)
>  create mode 100644 test/log_process.cpp
> 
> diff --git a/test/log_process.cpp b/test/log_process.cpp
> new file mode 100644
> index 0000000..feda687
> --- /dev/null
> +++ b/test/log_process.cpp
> @@ -0,0 +1,147 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * process_test.cpp - Process test

Forgot to rename ?

> + */
> +
> +#include <ctime>
> +#include <cstdlib>
> +#include <fcntl.h>
> +#include <iostream>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <vector>
> +
> +#include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/logging.h>
> +#include <libcamera/timer.h>
> +
> +#include "log.h"
> +#include "process.h"
> +#include "test.h"
> +#include "utils.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +static string message("hello from the child");
> +
> +LOG_DEFINE_CATEGORY(LogProcessTest)
> +
> +class LogProcessTestChild
> +{
> +public:
> +	int run(int status, int num)
> +	{
> +		usleep(50000);
> +
> +		string logPath = "/tmp/libcamera.worker.test." +
> +				 to_string(num) + ".log";
> +		if (logSetFile(logPath.c_str()) < 0)
> +			return TestSkip;
> +		LOG(LogProcessTest, Warning) << message;
> +
> +		return status;
> +	}
> +};
> +
> +class LogProcessTest : public Test
> +{
> +public:
> +	LogProcessTest()
> +	{
> +	}

You can drop the empty default constructor.

> +
> +protected:
> +	int run()
> +	{
> +		EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
> +		Timer timeout;
> +
> +		srand(time(0));
> +		int num = rand();

If you want to do it the C++ way,

		std::random_device random;
		int num = random();

> +
> +		int exitCode = 42;
> +		vector<std::string> args;
> +		args.push_back(to_string(exitCode));
> +		args.push_back(to_string(num));
> +		int ret = proc_.start("/proc/self/exe", args);
> +		if (ret) {
> +			cerr << "failed to start process" << endl;
> +			return TestFail;
> +		}
> +		proc_.finished.connect(this, &LogProcessTest::procFinished);

You should connect signals before calling the method that will result in
the signal being emitted, event if it is safe in this case. I would move
the connect and random number generation to the init() method of the
test.

> +
> +		timeout.start(200);
> +		while (timeout.isRunning())
> +			dispatcher->processEvents();
> +
> +		if (exitStatus_ != Process::NormalExit) {
> +			cerr << "process did not exit normally" << endl;
> +			return TestFail;
> +		}
> +
> +		if (exitCode_ == TestSkip)
> +			return TestSkip;

This shouldn't happen, that should thus be an error.

> +
> +		if (exitCode != exitCode_) {
> +			cerr << "exit code should be " << exitCode
> +			     << ", actual is " << exitCode_ << endl;
> +			return TestFail;
> +		}
> +
> +		string logPath = "/tmp/libcamera.worker.test." +
> +				 to_string(num) + ".log";

Can you make sure the file gets unlinked ? The best way is likely to
call unlink in the cleanup() method.

> +		int fd = open(logPath.c_str(), O_RDONLY, S_IRUSR);
> +		if (fd < 0) {
> +			cerr << "failed to open tmp log file" << endl;
> +			return TestFail;
> +		}
> +
> +		char buf[200];
> +		memset(buf, 0, sizeof(buf));
> +		lseek(fd, 0, SEEK_SET);

This shouldn't be necessary as you've just opened the file.

> +		if (read(fd, buf, sizeof(buf)) < 0) {
> +			cerr << "Failed to read tmp log file" << endl;
> +			return TestFail;
> +		}
> +		close(fd);
> +
> +		string str(buf);
> +		if (str.find(message) == string::npos)
> +			return TestFail;
> +
> +		return TestPass;
> +	}
> +
> +private:
> +	void procFinished(Process *proc, enum Process::ExitStatus exitStatus, int exitCode)
> +	{
> +		exitStatus_ = exitStatus;
> +		exitCode_ = exitCode;
> +	}
> +
> +	Process proc_;
> +	enum Process::ExitStatus exitStatus_;
> +	int exitCode_;
> +};
> +
> +/*
> + * Can't use TEST_REGISTER() as single binary needs to act as both
> + * parent and child processes.
> + */
> +int main(int argc, char **argv)
> +{
> +	if (argc == 3) {
> +		int status = std::stoi(argv[1]);
> +		int num = std::stoi(argv[2]);
> +		LogProcessTestChild child;
> +		return child.run(status, num);
> +	}
> +
> +	return LogProcessTest().execute();
> +}
> diff --git a/test/meson.build b/test/meson.build
> index ad1a2f2..658f283 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -23,6 +23,7 @@ public_tests = [
>  internal_tests = [
>      ['camera-sensor',                   'camera-sensor.cpp'],
>      ['log',                             'log.cpp'],
> +    ['log_process',                     'log_process.cpp'],
>      ['message',                         'message.cpp'],
>      ['signal-threads',                  'signal-threads.cpp'],
>      ['threads',                         'threads.cpp'],

-- 
Regards,

Laurent Pinchart