[ur] Improve logger env var handling #438

PatKamin · 2023-04-11T12:05:30Z

Add tests for validation of the global logger object setup from
an environment variable. Improve logger env var handling.

pbalcer · 2023-04-12T12:15:20Z

source/common/ur_util.hpp

+        auto is_quoted = [&value]() {
+            return (value.front() == '\'' && value.back() == '\'') ||
+                           (value.front() == '"' && value.back() == '"')
+                       ? true


no need for ? true : false.

Indeed, thanks.

pbalcer · 2023-04-12T12:16:27Z

cmake/match.cmake

-endif()
-if(NOT DEFINED MATCH_FILE)
-    message(FATAL_ERROR "MATCH_FILE needs to be defined")
+# Process variables passed to the script


I'm not sure I like it this way. I'd rather have just one way of building tests with match files. But let's leave it for now.

pbalcer · 2023-04-12T12:16:48Z

cmake/match.cmake

+# In 'nofile' mode, check if the OUT_FILE exists and exit the script
+if(${MODE} STREQUAL "nofile")
+    if(EXISTS ${OUT_FILE})
+        file(REMOVE ${OUT_FILE})


you souldn't remove random files like this.

Moved removal of files to tests set up. However, the output file is not removed after tests, rather before them, so the out file will exist after running the last test.

pbalcer · 2023-04-12T12:17:02Z

cmake/match.cmake

+        file(REMOVE ${OUT_FILE})
+        message(FATAL_ERROR "Failed: File ${OUT_FILE} should not be created")
+    else()
+        message("Passed: Default logger has been constructed")


make the message generic ;)

Good point :)

source/common/logger/ur_logger.hpp

test/unit/logger/CMakeLists.txt

PatKamin · 2023-04-14T13:55:36Z

Are you guaranteed that these tests are going to run in order? Why not just run the test in the match script?

There is no guarantee that they run in order, just that the first test is set to finish before this one. Between these two other tests can run. Hence, I restructured a bit tests to be run from a cmake wrapper script using execute_process() command.

pbalcer · 2023-04-17T13:32:54Z

cmake/match.cmake

-    message(FATAL_ERROR "TEST_FILE needs to be defined")
+# Process variables passed to the script
+if(NOT DEFINED MODE)
+    message(FATAL_ERROR "MODE needs to be defined. Possible values: 'binary', 'file'")


I think really the difference is what we match against, some specified file or stdout, right? If so, I think it would be better to call these modes 'stdout' and 'file'.

pbalcer · 2023-04-17T13:36:18Z

test/unit/logger/logfile_match_wrapper.cmake

+    file(REMOVE ${OUT_FILE})
+endif()
+
+execute_process(


I mean, this is exactly what the match.cmake does. So what you are effectively doing is creating a wrapper with the same functionality that is being disabled (by mode 'binary') in the main script?

Again, I'm not sure I fully understand, but if all you are trying to do is match against different output destination (stdout, stderr, file), then it'd be easier to create that argument in the original match script.

This wrapper removes also files before and after the test. I'm moving for now such a removal. It also allows for not having a file at all, in which case compare argument in cmake.match script would fail and I would have to have some other check introduced there (with ifdef) instead. Perhaps python script from #444 would simplify things, I'll rebase to it.

pbalcer · 2023-04-17T13:37:18Z

test/unit/logger/CMakeLists.txt

+        ENVIRONMENT ${env_var}\;GTEST_FILTER=*${test_case}\;GTEST_BRIEF=1
    )
+
+    file(READ ${match_file} MATCH_STRING)


I added support for regex matches in #444

Add tests for validation of the global logger object setup from an environment variable.

kswiecicki · 2023-04-20T11:12:58Z

source/common/ur_util.hpp

-    ex_ss << "Wrong format of the " << env_var_name << " environment variable!"
-          << " Proper format is: "
+    ex_ss << "Wrong format of the " << env_var_name
+          << " environment variable: '" << env_var_value << "'\n"


This wording might be a bit confusing.

Rephrased a bit

kswiecicki · 2023-04-20T11:25:15Z

source/common/ur_util.hpp

+    ex_ss << "Wrong format of the " << env_var_name
+          << " environment variable: '" << env_var_value << "'\n"
+          << "Proper format is: "
             "ENV_VAR=\"param_1:value_1,value_2;param_2:value_1";


There's \" missing at the end of the exception string.

kswiecicki · 2023-04-20T11:32:25Z

source/common/ur_util.hpp

-            if (value.empty()) {
-                throw_wrong_format_map(env_var_name);
+            if (value.empty() ||
+                ((value.find(':') != std::string::npos) && !is_quoted())) {


Since you have an anonymous function for is_quoted you might want to create has_colon for (value.find(':') != std::string::npos).

Added has_colon(). Also moved lambdas out of the while loop.

- catch an exception thrown by env var parsing function - check for unknown parameters set in an env var - fix wrong logger level when creating logger from an environment variable with an "error" level set - handle Windows absolute path to logfile properly - extend failure messages - strict check for standard output logger 'output' option

pbalcer reviewed Apr 12, 2023

View reviewed changes

igchor reviewed Apr 12, 2023

View reviewed changes

source/common/logger/ur_logger.hpp Show resolved Hide resolved

PatKamin force-pushed the logger-env-var-tests branch from 2a942f9 to 2871f48 Compare April 14, 2023 09:56

pbalcer reviewed Apr 14, 2023

View reviewed changes

test/unit/logger/CMakeLists.txt Show resolved Hide resolved

PatKamin force-pushed the logger-env-var-tests branch from 2871f48 to a538d96 Compare April 14, 2023 13:51

PatKamin force-pushed the logger-env-var-tests branch from a538d96 to 4b0af3f Compare April 14, 2023 13:58

pbalcer reviewed Apr 17, 2023

View reviewed changes

PatKamin force-pushed the logger-env-var-tests branch 3 times, most recently from 65fc267 to a0484b9 Compare April 20, 2023 10:17

Add global logger object tests

9cc7c7e

Add tests for validation of the global logger object setup from an environment variable.

kswiecicki reviewed Apr 20, 2023

View reviewed changes

PatKamin force-pushed the logger-env-var-tests branch from a0484b9 to 93754d2 Compare April 20, 2023 12:08

kswiecicki approved these changes Apr 20, 2023

View reviewed changes

pbalcer approved these changes Apr 20, 2023

View reviewed changes

pbalcer merged commit 1382907 into oneapi-src:main Apr 20, 2023

PatKamin deleted the logger-env-var-tests branch June 26, 2024 10:05

[ur] Improve logger env var handling #438

[ur] Improve logger env var handling #438

Uh oh!

Conversation

PatKamin commented Apr 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PatKamin commented Apr 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants