-
-
Notifications
You must be signed in to change notification settings - Fork 113
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
upgrade to 2.15 #243
upgrade to 2.15 #243
Conversation
e395887
to
126161e
Compare
@Dhaulagiri Think I could get a review? This PR should fix our failing tests and we are back to being green on every version in our |
); | ||
}); | ||
}); | ||
later(() => { |
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.
Rather than trying to be clever and use sinon
to control the time, we'll just force it with later
. It makes the test take a second longer to run, but we can get rid of sinon
and this better matches the actual behavior.
126161e
to
9703f2a
Compare
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.
Overall LGTM! Left a couple small comments/questions.
package.json
Outdated
@@ -25,32 +25,31 @@ | |||
], | |||
"license": "MIT", | |||
"dependencies": { | |||
"bower": "^1.8.2", |
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 think this should go in devdeps since this is only to get ember try working, right?
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.
Good call. Fixed
package.json
Outdated
}, | ||
"devDependencies": { | ||
"broccoli-asset-rev": "^2.4.5", | ||
"ember-ajax": "3.0.0", | ||
"ember-cli": "^2.14.1", | ||
"ember-ajax": "^3.0.0", |
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 think we could actually remove ember-ajax entirely here? it's part of the default blueprint but I don't think we are using it are we?
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.
Nope, good call.
9703f2a
to
42c9633
Compare
What's in this PR?
sinon
for stubbing methodscloses: #242