[libcamera-devel] [PATCH v2 2/2] test: add logging API test

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Thu, Jul 11, 2019 at 07:24:18PM +0900, Paul Elder wrote:
> Test that setting the log file and log levels works from an application
> point of view. The test uses the internal logging mechanism as well,
> just to write to the log file.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Changes in v2:
> - added more test cases to catch if log level changes fail
> 
>  test/log/log_api.cpp | 75 ++++++++++++++++++++++++++++++++++++++++++++
>  test/log/meson.build | 12 +++++++
>  test/meson.build     |  1 +

There's no need to create a directory for a single test, you can move it
to test/, and I would even call it log.cpp for now.

>  3 files changed, 88 insertions(+)
>  create mode 100644 test/log/log_api.cpp
>  create mode 100644 test/log/meson.build
> 
> diff --git a/test/log/log_api.cpp b/test/log/log_api.cpp
> new file mode 100644
> index 0000000..395b857
> --- /dev/null
> +++ b/test/log/log_api.cpp
> @@ -0,0 +1,75 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * log_api.cpp - log API test
> + */
> +
> +#include <algorithm>
> +#include <fcntl.h>
> +#include <iostream>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <libcamera/logging.h>
> +
> +#include "log.h"
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(LogAPITest)
> +
> +class LogAPITest : public Test
> +{
> +protected:
> +	int run() override
> +	{
> +		int fd = open("/tmp", O_TMPFILE | O_RDWR, S_IRUSR | S_IWUSR);
> +		if (fd < 0) {
> +			cerr << "Failed to open tmp log file" << endl;
> +			return TestFail;
> +		}
> +
> +		char path[PATH_MAX];

That's a bit long, 32 will largely suffice.

> +		snprintf(path, PATH_MAX, "/proc/self/fd/%d", fd);

s/PATH_MAX/sizeof(path)/
s/%d/%u/ as we know the number is positive

> +
> +		logSetFile(path);
> +
> +		logSetLevel("LogAPITest", "DEBUG");
> +		LOG(LogAPITest, Info) << "asdf";
> +
> +		logSetLevel("LogAPITest", "WARN");
> +		LOG(LogAPITest, Info) << "asdf";
> +
> +		logSetLevel("LogAPITest", "WARN");
> +		LOG(LogAPITest, Info) << "asdf";

Those two lines don't appear to do anything more than the two previous
ones.

> +		LOG(LogAPITest, Debug) << "asdf";
> +
> +		logSetLevel("LogAPITest", "ERROR");
> +		LOG(LogAPITest, Error) << "asdf";
> +		LOG(LogAPITest, Info) << "asdf";
> +
> +		logSetLevel("LogAPITest", "WARN");
> +		LOG(LogAPITest, Warning) << "asdf";
> +		LOG(LogAPITest, Info) << "asdf";

I would print a sequence of numbers instead of "asdf", and verify that
the expected numbers are printed. This will be easier if you read the
whole file (in a bigger array than 200 bytes) and wrap the buffer in an
istringstream
(https://en.cppreference.com/w/cpp/io/basic_istringstream). You can then
call getline() in a loop and get the last character of each line.

> +
> +		char buf[200];
> +		lseek(fd, 0, SEEK_SET);
> +		read(fd, buf, 1000);
> +		close(fd);
> +
> +		std::string s(buf);
> +		int n = count(s.begin(), s.end(), '\n');
> +		if (n == 3)
> +			return TestPass;
> +
> +		return TestFail;
> +	}
> +};
> +
> +TEST_REGISTER(LogAPITest)
> diff --git a/test/log/meson.build b/test/log/meson.build
> new file mode 100644
> index 0000000..35ea553
> --- /dev/null
> +++ b/test/log/meson.build
> @@ -0,0 +1,12 @@
> +log_api_test = [
> +    ['log_api', 'log_api.cpp'],
> +]
> +
> +foreach t : log_api_test
> +    exe = executable(t[0], t[1],
> +                     dependencies : libcamera_dep,
> +                     link_with : test_libraries,
> +                     include_directories : test_includes_internal)
> +
> +    test(t[0], exe, suite : 'log')
> +endforeach
> diff --git a/test/meson.build b/test/meson.build
> index 60ce960..44d8495 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -4,6 +4,7 @@ subdir('camera')
>  subdir('controls')
>  subdir('ipa')
>  subdir('ipc')
> +subdir('log')
>  subdir('media_device')
>  subdir('pipeline')
>  subdir('stream')

-- 
Regards,

Laurent Pinchart