Skip to content
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

Merged
merged 17 commits into from
Jun 20, 2018
Merged

Conversation

faustinoaq
Copy link
Contributor

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 template

Benefits

Print query logs (very useful for debugging specs)
Fix crystal spec on projects generated with -m crecto

Possible Drawbacks

No

@@ -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

Copy link
Contributor

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

Copy link
Contributor Author

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 😅

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! 👍

@eliasjpr
Copy link
Contributor

I think instead of removing the logger for Granite we should render the right logger for the right ORM WDYT?

@faustinoaq
Copy link
Contributor Author

faustinoaq commented Jun 14, 2018

@eliasjpr Yeah, I'm already fixing this 👍

Edit: Done 🎉 !

@faustinoaq faustinoaq self-assigned this Jun 15, 2018
@faustinoaq faustinoaq merged commit 3e84da0 into master Jun 20, 2018
@faustinoaq faustinoaq deleted the fa/fix-crecto-spec branch June 20, 2018 18:03
@faustinoaq
Copy link
Contributor Author

Required by #850

@faustinoaq faustinoaq added this to the Version 0.8.0 milestone Jun 21, 2018
@faustinoaq
Copy link
Contributor Author

@robacarp for some reason this is still generating Granite logs, any idea?

I checked twice and spec_helper.cr is generated with the code to disable logs, Are we enabling logs in some other file? maybe in some initializer?

Ref: https://travis-ci.org/amberframework/amber/jobs/394384754#L736

@faustinoaq
Copy link
Contributor Author

Opened tracking issue here: #885

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants