fix the logger and make it work

[mengwangme]

Fixes #1395

[codecov-io]

Codecov Report

Merging #1397 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1397      +/-   ##
==========================================
- Coverage    81.4%   81.34%   -0.06%     
==========================================
  Files          17       17              
  Lines        1694     1694              
  Branches      322      322              
==========================================
- Hits         1379     1378       -1     
  Misses        247      247              
- Partials       68       69       +1

Impacted Files	Coverage Δ
sanic/log.py	100% <ø> (ø)	⬆️
sanic/router.py	92.23% <0%> (-0.46%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3a27c2...32557c4. Read the comment docs.

[harshanarayana]

Would you be kind enough to modify the unit test test_logging_defaults to assert the default log level just so that issue like this will not creep into the release in the future?

[harshanarayana]

It may not be a bad idea to add an explicit test case to validate the default root logger level as well.

def test_logging_modified_root_logger_config():
    reset_logging()

    modified_config = LOGGING_CONFIG_DEFAULTS
    modified_config['loggers']['sanic.root']['level'] = "DEBUG"

    app = Sanic("test_logging", log_config=modified_config)

    assert logging.getLogger('sanic.root').getEffectiveLevel() == logging.DEBUG

[mengwangme]

@harshanarayana thanks, I didn't think enough, I will modify it right now.

[harshanarayana]

Something looks fishy in that coverage report. Why is sanic/router.py being reported in Coverage delta?

[ahopkins]

I'm on my phone so I it's a little hard to do a sufficient review, but with this in place and passing, we can close #1049?

[harshanarayana]

@ahopkins I think so. This PR addresses the same issue.

[chenjr0719]

@KeepitReal555 @harshanarayana I'm afraid It's more complicated about those tests and this is why I didn't send the PR immediately. Let me address some problem I found:

1. All tests in test_logging.py use root.
   This is the easiest problem, just replace root to sanic.root can fix.

2. Some tests use handcrafted logger(here and here).
   I'm afraid this may not behave the same result against the original logger in sanic.log.

3. Incorrect assertions.
   In test_logging.py, most assertions only validate stream output from those handcrafted logger(see here). I think this is the main reason why all the tests have been passed because it didn't assert the output from the original logger.

More directly, current tests in test_logging.py can't really verify the functionality of sanic.log. I will prefer to rewrite/refactor all tests in test_logging.py. Any idea?

[mengwangme]

Something looks fishy in that coverage report. Why is sanic/router.py being reported in Coverage delta?

I found out, but I don't konw why, maybe it's my mistake.

[mengwangme]

@chenjr0719 You're right, there are bugs in the tests, I should also think about it.

[harshanarayana]

@chenjr0719 Agreed. I am more than happy to help if we need to rewrite the test_logging.py.

As for the handcrafter loggers go, the existing tests can be retained. But we need to have a similar test case that can validate the output in case if you are using sanic.root with logging.getLogger. Would that make sense?

IMHO, I think it might be a good idea to create a different PR that addresses the necessary fix in the test_logging.py than extending this PR to include that change as well.

[chenjr0719]

@harshanarayana I like your idea, let me take a try, I will send another PR later.

[ahopkins]

I am adding my approval as this does the job of solving #1395 with the presumption that there will be another PR to beef up the tests.

[sjsadowski]

I'm going to merge. @chenjr0719 added #1400 in response to @ahopkins approval, and I'd like to get this in to 18.12.