<@U092308M7> Could you take a look at my test and ...

# ktor

[ddsoyka]

@orangy Could you take a look at my test and tell me if I did something wrong? Because the applicationStarting event never seems to get logged. There is also some weirdness WRT logger.isTraceEnabled reporting false even though logging succeeds.

[orangy]

I'll check it later today. Cooking for kids :)

[ddsoyka]

Okay, thanks

[orangy]

applicationStarting event is called before module functions are executed. So when application code is run (and installs CallLogging), it is already executed. Good catch! We need to think if it’s fine or not.

[orangy]

Currently hosts are using it to attach their handlers and install their interceptors into an application, including when application reloads

[ddsoyka]

I am thinking that I would like to create a separate PR for the unit tests, and then rebase the logging-improvements branch onto it. I want to do this because the tests are needed regardless of whether this PR is merged. Does this sound okay to you?

[ddsoyka]

https://github.com/ddsoyka/ktor/blob/call-logging-tests/ktor-core-tests/test/org/jetbrains/ktor/tests/http/CallLoggingTest.kt

[orangy]

You might want to set code style to import with * for this project 🙂

[orangy]

Also, I don’t think you really need CopyOnWriteArrayList there, calls in tests are sequential unless you specifically make them parallel

[orangy]

Otherwise looks good!

[ddsoyka]

Changes made, thanks.

[ddsoyka]

https://github.com/Kotlin/ktor/pull/200

[orangy]

Thanks! However assertEquals("Application starting: $hash", messages[1]) is still failing.

[orangy]

I mean for now let’s make it without starting, we’ll get to it later.

[ddsoyka]

Yeah... I wasn't sure whether to commit a failing test, I am very new to this

[ddsoyka]

My thinking was, it's better to have the test fail, than to change the test in such a way that it no longer represents expected behaviour?

[orangy]

The problem is it’s unclear what behaviour is expected at this moment.

[orangy]

The logic behind events is clear.
* application instance created
* starting
* all app code executed (modules)
* started
* app serving calls
* stopping
* all features removed, closables called, etc
* stopped

[orangy]

CallLogging, being an application feature, thus cannot participate in starting & stopped events. So the question is if we should make call logging something like “environment” feature (we don’t have such a thing yet), or ensure stopped is not called on it

[ddsoyka]

Okay, I fixed the test to match current behaviour.

[orangy]

Thanks!

[ddsoyka]

I understand... I wish I knew enough about the internals of Ktor to contribute a meaningful opinion here.

[orangy]

Merged, thanks!

[orangy]

I’m thinking lifecycle events shouldn’t be logged by this feature at all. They should be logged by host.

[orangy]

So, do I wait for rebase of the log-level branch and tests?

[ddsoyka]

Done; sorry, was having dinner.

[ddsoyka]

Is the current state of logging-improvements acceptable? It throws on illegal log level, rather than logging an error

[ddsoyka]

Actually, this raises an earlier issue I was having; the tests fail because Logger.isTraceEnabled is reporting false even though the logging statements are invoked properly... not sure what is going on here

[orangy]

I’ll check it tomorrow, or may be on Monday. It’s already 1:15 into Saturday night here 🙂

[ddsoyka]

Okay, no worries.