Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Fixed language manager unit test. Issue #3957 #4106

Merged
merged 4 commits into from
Jun 6, 2013
Merged

Fixed language manager unit test. Issue #3957 #4106

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
93c6d96
Fixed language manager unit test. Issue #3957
thefirstofthe300 Jun 5, 2013
18d8dd8
Removed phtml file extension.
thefirstofthe300 Jun 5, 2013
55bb6aa
Fixed logic errors and poorly written loop
thefirstofthe300 Jun 5, 2013
11bb449
Improved conciseness and quality of code
thefirstofthe300 Jun 6, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion test/spec/LanguageManager-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,26 @@ define(function (require, exports, module) {
expect(LanguageManager.getLanguage(expected.id)).toBe(actual);
}

var i = 0;
var expectedFileExtensions;

// Be sure that expectedFileExtensions is not undefined.
if (expected.fileExtensions === undefined) {
expectedFileExtensions = [];
} else {
expectedFileExtensions = expected.fileExtensions;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This block of code:

            var expectedFileExtensions;

            // Be sure that expectedFileExtensions is not undefined.  
            if (expected.fileExtensions === undefined) {
                expectedFileExtensions = [];
            } else {
                expectedFileExtensions = expected.fileExtensions;
            }

Can be replaced by this line of code:

            var expectedFileExtensions = expected.fileExtensions || [];

When initializing expectedFileExtensions, it's set to the first "truthy" element in the OR list. So, if expected.fileExtensions is undefined (or null), then it's set to [] (an empty array). This code is actually better because it checks for both undefined and null;

var expectedFileExtensionsLength = expectedFileExtensions.length;
var actualFileExtensions = actual.getFileExtensions();

expect(actual.getId()).toBe(expected.id);
expect(actual.getName()).toBe(expected.name);
expect(actual.getFileExtensions()).toEqual(expected.fileExtensions || []);

for (i; i < expectedFileExtensionsLength; i++) {
expect(actualFileExtensions).toContain(expectedFileExtensions[i]);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This code never goes in the loop because actual.getFileExtensions is a reference to a function, it's not actually calling the function. It happens to have a length property that's 0. You can click on Show Developer Tools from the Jasmine Spec Runner Window, the click on the Source tab and set a breakpoint in the code to see what branches it takes and inspect the structure and value of object. This should be actual.getFileExtensions().length instead.

Even better would be to only call actual.getFileExtensions() once and store it in a local var.


expect(actual.getFileNames()).toEqual(expected.fileNames || []);

if (expected.blockComment) {
Expand Down