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
Improved settings and fixed tests #364
Conversation
effecting it's state randomly. Also conforms to best practices.
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.
lgtm.
I think overall, we can continue to make an effort to have better structure the tests (files and actual tests).
I ran into a similar issue where it was a challenge to effectively isolate tests.
If there are any before_each
or after_each
we should try to get rid of them. They are unscoped - it is before_each test in the entire test suite.
@@ -37,6 +36,7 @@ describe Amber do | |||
end |
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.
It seems like this should be a test for Amber::Settings
rather than Amber::Server.configure
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.
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.
Approved after @marksiemers comment
* Fixed session so that it could be tested without previous tests effecting it's state randomly. Also conforms to best practices. * resolved logging level settings with instance variables * added settings to configure with block since it's probably clear than configure. * added requested test change
* Fixed session so that it could be tested without previous tests effecting it's state randomly. Also conforms to best practices. * resolved logging level settings with instance variables * added settings to configure with block since it's probably clear than configure. * added requested test change Former-commit-id: 232ab29
Description of the Change
Fixed Settings so that it is initialized. This allows it to be tested without previous tests effecting it's state randomly. Also conforms to best practices.
Benefits
Allows for better tests. A few of the settings tests had to be commented out because settings were being overwritten by previous tests. It also makes more sense to instantiate settings when server is started instead of having a preexisting pseudo singleton.
Possible Drawbacks
None.