Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streamline Logging Across TestNG #2646

Closed
krmahadevan opened this issue Oct 5, 2021 · 5 comments · Fixed by #2698
Closed

Streamline Logging Across TestNG #2646

krmahadevan opened this issue Oct 5, 2021 · 5 comments · Fixed by #2698

Comments

@krmahadevan
Copy link
Member

TestNG Version

Note: only the latest version is supported

7.4.0

Currently TestNG uses a mixture of sysouts and a custom logger that only logs to the console. We should enhance TestNG to use slf4j api integration so that our downstream consumers can basically work with their logging configurations and also help them streamline the test logs.

@juherr WDYT ?

@juherr
Copy link
Member

juherr commented Oct 5, 2021

Sure, I think slf4j is light enough to be added as a dependency.

From the architecture pov, I think we should keep the current logger facade and call slf4j from there.

@krmahadevan
Copy link
Member Author

@juherr - Cool. I will work on raising a PR that addresses this ask shortly

@juherr
Copy link
Member

juherr commented Oct 11, 2021

TBD: how should be the verbose level managed?
From #2654 (comment)

@krmahadevan
Copy link
Member Author

@juherr - I think we should think of deprecating the verbose attribute and just have it managed via the loggers verbosity. The other option would be to try and map a few of the initial values to the loggers verbosity (1 - error, 2 - warning) but I think that's going to be counter intuitive but will help us retain backward compatibility.

@juherr
Copy link
Member

juherr commented Oct 12, 2021

I don't know if we can remove the verbose level because it could be used by default listeners.
For sure, it should be possible to override the value of every listener.

But I agree that the internal logger should not be impacted by this value (but it could be by the runner configuration).

The level map will be necessary for the support of Reporter.log().

krmahadevan added a commit to krmahadevan/testng that referenced this issue Dec 28, 2021
Closes testng-team#2646

We now use sl4j apis for our logging.
Internally TestNG makes use of SimpleLogger 
(Slf4j-simple) bindings for logging purposes.
krmahadevan added a commit to krmahadevan/testng that referenced this issue Dec 28, 2021
Closes testng-team#2646

We now use sl4j apis for our logging.
Internally TestNG makes use of SimpleLogger 
(Slf4j-simple) bindings for logging purposes.
@krmahadevan krmahadevan added this to the 7.5.0 milestone Dec 28, 2021
krmahadevan added a commit to krmahadevan/testng that referenced this issue Dec 28, 2021
Closes testng-team#2646

We now use sl4j apis for our logging.
Internally TestNG makes use of SimpleLogger 
(Slf4j-simple) bindings for logging purposes.
krmahadevan added a commit to krmahadevan/testng that referenced this issue Dec 29, 2021
Closes testng-team#2646

We now use sl4j apis for our logging.
Internally TestNG makes use of SimpleLogger 
(Slf4j-simple) bindings for logging purposes.
krmahadevan added a commit to krmahadevan/testng that referenced this issue Dec 31, 2021
Closes testng-team#2646

We now use sl4j apis for our logging.
Internally TestNG makes use of SimpleLogger 
(Slf4j-simple) bindings for logging purposes.
krmahadevan added a commit to krmahadevan/testng that referenced this issue Dec 31, 2021
Closes testng-team#2646

We now use sl4j apis for our logging.
Internally TestNG makes use of SimpleLogger 
(Slf4j-simple) bindings for logging purposes.
krmahadevan added a commit that referenced this issue Dec 31, 2021
Closes #2646

We now use sl4j apis for our logging.
Internally TestNG makes use of SimpleLogger 
(Slf4j-simple) bindings for logging purposes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants