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

feat(#973): Allow deactivation of standard LoggingReporter #975

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

tschlat
Copy link
Collaborator

@tschlat tschlat commented Sep 7, 2023

I came up with a solution that enables the complete disablement of the LoggingReporter's functionality. Considering that both the HtmlReporter and JUnitReporter already offer similar disablement options, I believe this solution adequately addresses the requirements.

@tschlat tschlat self-assigned this Sep 7, 2023
@tschlat tschlat requested a review from bbortt September 7, 2023 13:24
@tschlat tschlat linked an issue Sep 7, 2023 that may be closed by this pull request
* @param enabled
*/
public void setEnabled(boolean enabled) {
this.enabled = enabled;
Copy link
Collaborator

@bbortt bbortt Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a possibility to completly supress the underlying logger? because you'd have to add an immediate return to every method - also info and error etc. that seems a bit much to me....

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or we do say that in the case of a direct invocation it might be wanted and we do knowingly not suppress it. but that clashes with the current implementation of the debug method.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

@tschlat tschlat Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a possibility to completly supress the underlying logger? because you'd have to add an immediate return to every method - also info and error etc. that seems a bit much to me....

Please see the javadoc of the class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this https://www.slf4j.org/api/org/slf4j/helpers/NOPLogger.html can help?

Gratefully accepted.

@tschlat tschlat force-pushed the issue/973/standard-reporter-override-2-TS branch from 79b62e6 to 67532c8 Compare September 7, 2023 14:07
@bbortt bbortt merged commit 4058578 into main Sep 8, 2023
1 check passed
@bbortt bbortt deleted the issue/973/standard-reporter-override-2-TS branch September 8, 2023 04:42
@christophd
Copy link
Member

christophd commented Sep 8, 2023

hey folks, thanks for the contribution!

I think it would also be nice to enable/disable the reporter via env var settings like HTMLReporter is doing (see HtmlReporter)

Also in general for all reporters it would be good to find a way to completely remove the reporter from the list of reporters once disabled. So we do not even initialize the reporter and send log events to it once disabled.

Reporters are initialized in DefaultTestReporters. So we should make this configurable and enable/disable reporters here in order to not even initialize the disabled ones.

For Spring configuration we can use

@ConditionalOnProperty(
  value="citrus.default.logging.enabled", 
  havingValue = "true", 
  matchIfMissing = true)

on the reporter bean to not even initialize the bean based on the env var settings.

I think I will open a separate enhancement issue for this feature.

@tschlat
Copy link
Collaborator Author

tschlat commented Sep 8, 2023

I implemented it like this, running into the problem, that the LogginReporter also serves as Logger for Inbound/Outbound messages.

You may want to have a look at this:
main...issue/973/standard-reporter-override-TS

The point is, that the default reporters which are registered only if required, are not available as beans and are thus not autowired properly. Especially with respect to the additional interfaces, the LoggingReporter is implementing.
I then dropped this approach.

I also already checked @ConditionalOnProperty. It would require the following dependency to be added to Citrus:

<dependency>
      <groupId>org.springframework.boot</groupId>
      <artifactId>spring-boot-autoconfigure</artifactId>
</dependency> 

If we want to avoid it, I could add respective conditions and use @conditional.

I'd be pleased to provide an MR for the conditional approach if you could shortly comment on the depencency question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow standard Reporter overrides
3 participants