Skip to content
This repository has been 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

merged 4 commits into from
Jun 6, 2013

Conversation

thefirstofthe300
Copy link
Contributor

This for (Issue #3957.

Okay. I am opening this new pull request to fix all of the merge issues of my last pull request (Issue #4097).

The code here takes all of the array of expected file extensions and makes sure it contains the actual file extensions, thus doing away with the problem of having to make the file extension arrays match.

I have tested this under my Windows VM and everything appears to work.

expect(actual.getFileExtensions()).toEqual(expected.fileExtensions || []);
for (i = 0; i < actual.getFileExtensions.length; i++) {
expect(expected.fileExtensions).toContain(actual.getFileExtensions()[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.

@redmunds
Copy link
Contributor

redmunds commented Jun 5, 2013

Done with initial code review.

@thefirstofthe300
Copy link
Contributor Author

Made sure the loop wasn't creating variables or calling functions over and over.

Also fixed logic.

Tested this under Win7 VM and everything appears to work. Dev tools say that the file extensions arrays are good too.

} 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;

@redmunds
Copy link
Contributor

redmunds commented Jun 6, 2013

This looks much better. One last comment.

@thefirstofthe300
Copy link
Contributor Author

I shortened up the code as suggested.

How does it look now?

@redmunds
Copy link
Contributor

redmunds commented Jun 6, 2013

Thanks. Merging.

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

Successfully merging this pull request may close these issues.

2 participants