[libcamera-devel] [PATCH 2/4] test: logging: add logSetStream test

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sat, Jul 13, 2019 at 05:16:18AM +0900, Paul Elder wrote:
> Test the new logSetStream loggin API call. Reorganize the logging API

s/loggin/logging/

> tests at the same time.

As for 1/4, it would probably have been easier to review if you had
split the rework into a preparatory patch.

> logSetTarget is not tested since the nowhere and syslog logging
> destinations do not really need to be tested.

I would test logSetTarget(LoggingTargetNone) and verify logging a
message doesn't crash, and then that logSetTarget(LoggingTargetFile) and
logSetTarget(LoggingTargetStream) both fail.

> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  test/log.cpp | 82 +++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 58 insertions(+), 24 deletions(-)
> 
> diff --git a/test/log.cpp b/test/log.cpp
> index 89fb5ca..c1446bd 100644
> --- a/test/log.cpp
> +++ b/test/log.cpp
> @@ -29,19 +29,8 @@ LOG_DEFINE_CATEGORY(LogAPITest)
>  class LogAPITest : public Test
>  {
>  protected:
> -	int run() override
> +	void doLogging()
>  	{
> -		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[32];
> -		snprintf(path, sizeof(path), "/proc/self/fd/%u", fd);
> -
> -		logSetFile(path);
> -
>  		logSetLevel("LogAPITest", "DEBUG");
>  		LOG(LogAPITest, Info) << "good 1";
>  
> @@ -55,19 +44,13 @@ protected:
>  		logSetLevel("LogAPITest", "WARN");
>  		LOG(LogAPITest, Warning) << "good 5";
>  		LOG(LogAPITest, Info) << "bad";
> +	}
>  
> -		char buf[1000];
> -		memset(buf, 0, sizeof(buf));
> -		lseek(fd, 0, SEEK_SET);
> -		if (read(fd, buf, sizeof(buf)) < 0) {
> -			cerr << "Failed to read tmp log file" << endl;
> -			return TestFail;
> -		}
> -		close(fd);
> -
> -		std::list<int> goodList = { 1, 3, 5 };
> -		std::basic_istringstream<char> iss((std::string(buf)));
> -		std::string line;
> +	int verifyOutput(string &str)
> +	{
> +		list<int> goodList = { 1, 3, 5 };
> +		basic_istringstream<char> iss(str);

You can use istringstream instead of basic_istringstream<char>.

> +		string line;
>  		while (getline(iss, line)) {
>  			if (goodList.empty()) {
>  				cout << "Too many log lines" << endl;
> @@ -90,6 +73,57 @@ protected:
>  
>  		return TestPass;
>  	}
> +
> +	int testFile()
> +	{
> +		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[32];
> +		snprintf(path, sizeof(path), "/proc/self/fd/%u", fd);
> +
> +		logSetFile(path);

You should return an error if logSetFile() fails (and print a message).

> +
> +		doLogging();
> +
> +		char buf[1000];
> +		memset(buf, 0, sizeof(buf));
> +		lseek(fd, 0, SEEK_SET);
> +		if (read(fd, buf, sizeof(buf)) < 0) {
> +			cerr << "Failed to read tmp log file" << endl;
> +			return TestFail;
> +		}
> +		close(fd);
> +
> +		string str(buf);
> +		return verifyOutput(str);
> +	}
> +
> +	int testStream()
> +	{
> +		ostringstream log;
> +		logSetStream(log);
> +
> +		doLogging();
> +
> +		string str(log.str());

How about passing istream to verifyOutput() ? You could then avoid
turning the string into an istringstream internally. You will need to
turn ostringstream into a stringstream. For the file case you would
create the istringstream in testFile().

> +		return verifyOutput(str);
> +	}
> +
> +	int run() override
> +	{
> +		int ret1 = testFile();
> +
> +		int ret2 = testStream();
> +
> +		if (ret1 == TestPass && ret2 == TestPass)
> +			return TestPass;

I think you can return failures as soon as they occur.

> +
> +		return TestFail;

Let's revert the condition to return TestSuccess at the end, like all
other tests.

> +	}
>  };
>  
>  TEST_REGISTER(LogAPITest)

-- 
Regards,

Laurent Pinchart