Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getEditsForFileRename: Test both before and after the rename #25074

Merged
2 commits merged into from
Jun 26, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jun 19, 2018

Fixes #25059 and #25029

Updates the fourslash tests to run once before the rename, perform the rename, then run again. The bug in #25059 only reproduced if getEditsForFileRename was called after the rename, which we weren't testing before.

@ghost ghost requested review from rbuckton and sheetalkamat June 19, 2018 18:35
@ghost ghost force-pushed the getEditsForFileRename_testBothWays branch from ae7e48a to f2bbb3a Compare June 19, 2018 19:24
@ghost ghost force-pushed the getEditsForFileRename_testBothWays branch from f2bbb3a to a5a0676 Compare June 19, 2018 20:02
export function getModuleSpecifier(compilerOptions: CompilerOptions, fromSourceFile: SourceFile, fromSourceFileName: string, toFileName: string, host: ModuleSpecifierResolutionHost, preferences: ModuleSpecifierPreferences = {}) {
const info = getInfo(compilerOptions, fromSourceFile, fromSourceFileName, host);
return getGlobalModuleSpecifier(toFileName, info, host, compilerOptions) ||
// Note: importingSourceFile is just for usesJsExtensionOnImports
Copy link
Contributor

Choose a reason for hiding this comment

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

should that instead be a boolean flag?

Copy link
Author

Choose a reason for hiding this comment

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

Let's do that in #25073 which is making this configurable.

Copy link
Contributor

@mhegazy mhegazy left a comment

Choose a reason for hiding this comment

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

@sheetalkamat can you take a look

@@ -16,6 +16,9 @@
//// "exclude": ["old"],
////}

// @Filename: /src/old/someFile.ts
////
Copy link
Member

Choose a reason for hiding this comment

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

With this change it seems we need to have oldFile present on the disk? What about the tests when getEditsForRename when oldFile isnt present on the disk?

Copy link
Author

Choose a reason for hiding this comment

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

For getEditsForFileRename to work, either oldFile xor newFile should be present. The tests start with oldFile present, verify the output of getEditsForFileRename, then rename the file, and verify the output of getEditsForFileRename again.

@ghost ghost merged commit ab9e913 into master Jun 26, 2018
@ghost ghost deleted the getEditsForFileRename_testBothWays branch June 26, 2018 23:55
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants