Remove Granite.settings.logger from spec#816
Conversation
|
|
||
| # Disable query logger for tests | ||
| Granite.settings.logger = Logger.new nil | ||
|
|
There was a problem hiding this comment.
Maybe we can set it dynamically? Take a look here
There was a problem hiding this comment.
@veelenga yeah, but I think Granite logs are very useful, you can see query time and raw sql information, these logs helped me to fix some issues on Amber itself 😅
There was a problem hiding this comment.
I'd prefer to leave it disabled because it generates so much noise. A verbose query log makes it hard to tell what happens when a test is failing. When tests all pass, the output of the suite should be minimal, indicating that nothing unexpected happened.
There was a problem hiding this comment.
@robacarp Crecto spec fails because granite dependency is missing on crecto projects. Maybe we should use a spec_helper.cr.ecr template as well, like this:
<%- if database == "granite" %>
Granite.settings.logger = Logger.new nil
<% end %>WDYT?
There was a problem hiding this comment.
@faustinoaq what happened here? Did the idea for the conditional get scrapped?
There was a problem hiding this comment.
@robacarp Yeah, let me add this conditional code 👍
|
I think instead of removing the logger for Granite we should render the right logger for the right ORM WDYT? |
|
@eliasjpr Yeah, I'm already fixing this 👍 Edit: Done 🎉 ! |
…into fa/fix-crecto-spec
…into fa/fix-crecto-spec
|
Required by #850 |
|
@robacarp for some reason this is still generating Granite logs, any idea? I checked twice and Ref: https://travis-ci.org/amberframework/amber/jobs/394384754#L736 |
|
Opened tracking issue here: #885 |
Description of the Change
This PR removes
Granite.settings.loggerfrom spec because causes compilation error on projects generated with-m crectoAlternate Designs
Use
spec_helper.cr.ecrtemplateBenefits
Print query logs (very useful for debugging specs)
Fix
crystal specon projects generated with-m crectoPossible Drawbacks
No