-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Fix crawlComplete exception when running tests #12659
Conversation
The change itself LGTM, but I'm not sure if instead we should always have the project name at this point in the code. |
@@ -96,7 +96,12 @@ define(function (require, exports, module) { | |||
} | |||
|
|||
function nodeFileCacheComplete(event, numFiles, cacheSize) { | |||
var projectName = ProjectManager.getProjectRoot().name || "noName00"; | |||
var projectName = ProjectManager.getProjectRoot(); |
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.
Small nit / bikeshedding: instead of assigning the return value getProjectRoot()
to projectName
, it could be a bit cleaner if it was something like:
var projectRoot = ProjectManager.getProjectRoot();
var projectName = projectRoot ? projectRoot.name : null;
if (!projectName) {
projectName = "noName00";
}
In general I think that there should never be an situation where projectRoot.name
is empty anyways until Brackets supports opening up with no projects at all.
I agree with @ficristo, there really shouldn't be any situation where there wasn't a |
d911ab3
to
4a5eb4a
Compare
To me, it looks like the Node instance sends the event both to the test window and the Jasmine window, where obviously, no project is loaded. So it runs fine in the test window, but throws this exception in the parent Test Runner window. |
@marcelgerber to me you are the expert so if you think is fine it's ok for me.
|
So, I just made sure my assumption is correct: Both the Test Runner and the test window receive an event in this case, and the Test Runner cannot handle it properly. I'm not sure whether it's the same event though (I think it is). One way would be (assuming it is one event) for the Test Runner to ignore all Node events it is being sent, which is definitely the more general solution and may prevent other issues as well. On the other hand, I'm not sure if some Node events are expected for tests to pass. I'll look into it. |
4a5eb4a
to
653ac8c
Compare
So, it turns out the SpecRunner definitely needs to receive Node events. |
@@ -93,7 +93,19 @@ define(function (require, exports, module) { | |||
} | |||
|
|||
function nodeFileCacheComplete(event, numFiles, cacheSize) { | |||
var projectName = ProjectManager.getProjectRoot().name || "noName00"; | |||
if (/\/test\/SpecRunner\.html$/.test(window.location.pathname)) { |
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.
Would be nice to check for the /brackets/i
regex too. But I don't know the location on the other OS.
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.
We cannot be certain what the git clone is named on the system. Also, it's very likely that a Brackets Shell window that has a SpecRunner.html loaded, which in turn loaded the rest of Brackets, actually belongs to Brackets ;)
I left a couple of comments, but otherwise LGTM. |
LGTM too 👍 |
Fixes #12658. (cc @ficristo)
Turns out the fix is quite straightforward.