EnvVariablesServerImpl constructor shouldn't unconditionally log

[jcortell68]

Bug Description:

The constructor of EnvVariablesServerImpl unconditionally does a console.log(); see code here

This is just bad practice. I get the value the author tried to provide, but applications should be in control of informational logging and not have it forced on them by the platform or by libraries. I can also see the the viewpoint that it's one statement that's logged at startup--what's the harm? In that scenario, there really is not much harm at all. But now consider an extensive unit test suite with many test functions that end up instantiating that object. Each such test now writes that line out. Tests normally don't write anything out. And so now you have a test report with, potentially, dozens or hundreds of lines of this noise.

Can the tests suppress the writes? Sure. We can and have taken measures to deal with this situation, but it's non-trivial and hackish. And ultimately, it shouldn't be necessary. I'm hoping this can be addressed in Theia so we can remove the mitigation in our test framework.

Steps to Reproduce:

N/A

Additional Information

N/A

[JonasHelming]

@martin-fleck-at

[martin-fleck-at]

@jcortell68 I see how console logging may seem like bad practice and you are very right in general. However, in Theia it is actually the recommended way if you do root-level logging because we actually intercept the console logging and re-direct it to an ILogger instance. This instance is backed by the console logger server by default and properly considers the default log level and custom log levels that can be configured through CLI arguments.

So we could argue whether the logging in the EnvVariablesServerImpl actually has the right log-level (log/info) but what you are saying is that the problem occurs in tests for you. As far as I can tell, the replacement of the generic console.log only happens when you actually use the loggerBackendModule which is not recommended for tests. Instead you could simply call bindLogger in your DI test setup. If this is not an option, I think it should be enough to simply call unsetRootLogger after the backend application has initialized, as the loggerBackendModule calls the setRootLogger function which does the override. However, removing the override simply means that we get the original console logging back. Alternatively, we could also just set different levels for the default logger and specific topics and in the worst case we need to bind our own console logger server that only allows specific messages.

I hope this makes it a bit clearer how things are happening in Theia and what could be done. If you elaborate on your use case in a bit more detail, I may be able to suggest a more concrete solution.

[jcortell68]

Thanks @martin-fleck-at . We'll look into your suggestions and report back soon.

[jcortell68]

@martin-fleck-at The tests I was referring to are jest unit tests. They are not end-to-end/Playwright tests that exercise the full Theia app environment. These unit tests don't involve the root logging interceptor. They bind the bare minimum needed by the functionality being tested.

Anyway, knowing that console is being used by design/intent with supporting code making it acceptable, I'll go ahead and close this ticket. Thanks for the insight!