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

Fix UpdateNotification.Locale Fallback tests #7819

Merged
merged 1 commit into from May 15, 2014

Conversation

Projects
None yet
3 participants
@MarcelGerber
Contributor

MarcelGerber commented May 11, 2014

As explained in #7811, the tests for that particular feature are succeeding right now while they shouldn't.
This fixes it - they'll fail right now on master, but as soon as release is merged back into master (including b325668), they'll work again.
@ingorichter

@@ -218,13 +218,16 @@ define(function (require, exports, module) {
function setupAjaxSpy(defaultUpdateUrl) {
var jq = spyOn(testWindow.$, "ajax").andCallFake(function (req) {
var d = $.Deferred();
var d = new $.Deferred();

This comment has been minimized.

@ingorichter

ingorichter May 12, 2014

Contributor

little nit: new is optional.

@ingorichter

ingorichter May 12, 2014

Contributor

little nit: new is optional.

This comment has been minimized.

@MarcelGerber

MarcelGerber May 12, 2014

Contributor

Well, it doesn't matter, right? I just did a quick search through the code and saw that mostly we use new $.Deferred();

@MarcelGerber

MarcelGerber May 12, 2014

Contributor

Well, it doesn't matter, right? I just did a quick search through the code and saw that mostly we use new $.Deferred();

This comment has been minimized.

@peterflynn

peterflynn May 12, 2014

Member

Our style guide explicitly says we should always use new

@peterflynn

peterflynn May 12, 2014

Member

Our style guide explicitly says we should always use new

This comment has been minimized.

@ingorichter

ingorichter May 15, 2014

Contributor

Yeah, and roughly 50% of our code doesn't use it either. I agree that we should keep it consistent.

@ingorichter

ingorichter May 15, 2014

Contributor

Yeah, and roughly 50% of our code doesn't use it either. I agree that we should keep it consistent.

ingorichter added a commit that referenced this pull request May 15, 2014

Merge pull request #7819 from SAPlayer/update-notification-test
Fix UpdateNotification.Locale Fallback tests

@ingorichter ingorichter merged commit fb64ff0 into adobe:master May 15, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@MarcelGerber MarcelGerber deleted the MarcelGerber:update-notification-test branch May 15, 2014

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn May 15, 2014

Member

@ingorichter Is this ready to merge or did you not do a full formal code review on it yet?

Member

peterflynn commented May 15, 2014

@ingorichter Is this ready to merge or did you not do a full formal code review on it yet?

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