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

fixes 3 failing find-in-files tests #12973

Merged
merged 2 commits into from
Dec 13, 2016
Merged

fixes 3 failing find-in-files tests #12973

merged 2 commits into from
Dec 13, 2016

Conversation

zaggino
Copy link
Contributor

@zaggino zaggino commented Dec 9, 2016

this PR fixes:

should add matches for a new file
should remove matches for a deleted file
should remove matches for a deleted folder

on my windows machine
ref: #12951

@zaggino zaggino requested a review from ficristo December 9, 2016 03:05
@zaggino zaggino mentioned this pull request Dec 9, 2016
Closed
ficristo
ficristo reviewed Dec 10, 2016
View reviewed changes
test/spec/FindInFiles-test.js
@@ -785,6 +785,7 @@ define(function (require, exports, module) {
gotChange = false;
oldResults = null;
wasQuickChange = false;
FindInFiles.searchModel.clear();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe call FindInFiles.clearSearch? It is marked as private method but it seems it is already used outside that file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we're calling FindInFiles.searchModel.on on the next line, I think calling FindInFiles.searchModel.clear directly is more clearer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have strong opinion here and I agree is it a bit odd.
My only doubt is clearSearch reset also findOrReplaceInProgress: I'm not sure how much it is important.

@ficristo
Copy link
Collaborator

The test should close the panel and detach listeners if a file is modified on disk is still failing for me.
I suppose it needs a different fix.

should do a case-sensitive search/replace from find bar was failing even before, so it's fine (for now).

@zaggino
Copy link
Contributor Author

zaggino commented Dec 10, 2016

Yeah, one test at a time I guess... I'll check what can fix the others next week when I'll have some free time.

@zaggino
Copy link
Contributor Author

zaggino commented Dec 12, 2016 •
edited
Loading

@ficristo ready to merge this one, i'll open another if I manage to fix other tests

@ficristo ficristo merged commit 1936493 into master Dec 13, 2016
@ficristo ficristo deleted the zaggino/fix-fs-tests branch December 13, 2016 19:42
This was referenced Aug 30, 2021
Open
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ficristo ficristo ficristo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zaggino @ficristo