-
Notifications
You must be signed in to change notification settings - Fork 207
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
Remove Granite.settings.logger from spec #816
Conversation
@@ -11,9 +11,6 @@ Micrate::DB.connection_url = Amber.settings.database_url | |||
# Automatically run migrations on the test database | |||
Micrate::Cli.run_up | |||
|
|||
# Disable query logger for tests | |||
Granite.settings.logger = Logger.new nil | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can set it dynamically? Take a look here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@faustinoaq yes, thats a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@faustinoaq what happened here? Did the idea for the conditional get scrapped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robacarp Yeah, let me add this conditional code 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! 👍
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 🎉 ! |
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.logger
from spec because causes compilation error on projects generated with-m crecto
Alternate Designs
Use
spec_helper.cr.ecr
templateBenefits
Print query logs (very useful for debugging specs)
Fix
crystal spec
on projects generated with-m crecto
Possible Drawbacks
No