-
Notifications
You must be signed in to change notification settings - Fork 7.5k
Fix language manager test (Issue #3957) #4097
Fix language manager test (Issue #3957) #4097
Conversation
|
@DaBungalow Thanks for taking this one on! It's frustrating when unit tests pass/fail randomly, so I randomly choosing 1 file extension from list is not a good idea. I think the test should loop through all file extensions in the test to verify that they are defined in Brackets. Then we can track down the exact ones that fail. Also, when I look in the Files Changed tab, I see changes to a lot of files that I don't think you changed, so I think this may be a bad merge. It might be easiest to start with a new branch and a new pull request. |
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 don't think Math.floor() * expected.fileExtensions.length achieves the goal of generating a random index into the array. I already said that we don't want any randomness in testing, but, for the sake of posterity, I think Math.floor(Math.random() * expected.fileExtensions.length) will do what you originally wanted. Note that this will try to access index 0 of an empty array, so that case may need to be isolated.
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.
That's what I was missing. I thought that didn't look right.
|
Yeah. You are right. I think I messed up the merge from upstream. Oops. Still getting the hang of the Git workflow. I now have a Win7 virtual machine running and I am having duplicating the error trouble. Can you explain what you do to get the error? That would help me solve the problem. |
|
The test is currently passing. The problem is that any time a new file extension is added to the HTML mode, the test fails. We want it to continue to pass. An easy way to see this is to temporarily remove one of the file extensions from the fileExtensions array for html mode. |
|
I have now opened a new pull request with just the LanguageManager-test.js file changed. #4106 |
Due to the difficulties of getting Brackets to even run on a Linux machine, I have not been able to test this code for unit testing.
I think that this code should fix the problem by testing to see if a random file extension exists in the file extension's array instead of testing to see if the arrays are identical.