-
Notifications
You must be signed in to change notification settings - Fork 406
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
fix: flushFilePath() with fix for Windows #4362
Changes from 1 commit
08ed3c5
a4427fc
da8080c
84a48d7
f632691
02458aa
bf78cf3
b172579
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,3 @@ | ||
|
||
/* | ||
* Copyright (c) 2019, salesforce.com, inc. | ||
* All rights reserved. | ||
|
@@ -7,20 +6,99 @@ | |
*/ | ||
|
||
import { expect } from 'chai'; | ||
import { extractJsonObject } from '../../../src/helpers'; | ||
import * as fs from 'fs'; | ||
import * as sinon from 'sinon'; | ||
import { | ||
extractJsonObject, | ||
flushFilePath, | ||
flushFilePaths | ||
} from '../../../src/helpers'; | ||
|
||
describe('getConfigSource', () => { | ||
|
||
it('should extract a JSON string from larger string and then return as an object', async () => { | ||
|
||
const exampleJsonString = JSON.stringify({ name: 'exampleName', error: 'exampleError' }); | ||
const exampleJsonString = JSON.stringify({ | ||
name: 'exampleName', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: there's a lot of format changes here in this PR. This was not my doing. When I save files, they are automatically being re-formatted. I'm not sure if there was a recent configuration change, but it seems like we have a lot of formatting changes in PRs lately. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've seen this also. Checked in w Gordon about it yesterday. We're going to create a ticket to look into why this is happening. |
||
error: 'exampleError' | ||
}); | ||
const exampleString = `junk text <junk +text junk text ${exampleJsonString} junk text junk text junk text`; | ||
|
||
const testParse = extractJsonObject(exampleString); | ||
|
||
expect(testParse.name).to.equal('exampleName'); | ||
expect(testParse.error).to.equal('exampleError'); | ||
}); | ||
}); | ||
|
||
describe('flushFilePath', () => { | ||
it('should call fs.realpathSync.native() to resolve a path', async () => { | ||
const filePath = 'C:\\Users\\temp\\exampleFile.js'; | ||
const realpathSyncNativeStub = sinon | ||
.stub(fs.realpathSync, 'native') | ||
.returns(filePath); | ||
|
||
const result = flushFilePath(filePath); | ||
|
||
expect(realpathSyncNativeStub.called).to.equal(true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since calledOnce is used below, is the expect for "called" needed here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wrote this a while ago... I think at the time my thought was, _"test & verify that it was called... ...AND test & verify that it was only called once"..., but I think you are right, in that it might be a bit redundant. I'll go & remove it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 nice catch |
||
expect(realpathSyncNativeStub.calledOnce).to.equal(true); | ||
expect(realpathSyncNativeStub.args[0][0]).to.equal(filePath); | ||
|
||
realpathSyncNativeStub.restore(); | ||
}); | ||
|
||
it('should return a path when a path is passed in', async () => { | ||
const filePath = 'C:\\Users\\temp\\exampleFile.js'; | ||
const realpathSyncNativeStub = sinon | ||
.stub(fs.realpathSync, 'native') | ||
.returns(filePath); | ||
|
||
const result = flushFilePath(filePath); | ||
|
||
expect(result).to.equal(filePath); | ||
|
||
realpathSyncNativeStub.restore(); | ||
}); | ||
|
||
it('should return an empty string when an empty sting is passed in', async () => { | ||
const filePath = 'c:\\Users\\temp\\exampleFile.js'; | ||
const realpathSyncNativeStub = sinon | ||
.stub(fs.realpathSync, 'native') | ||
.returns(''); | ||
|
||
const result = flushFilePath(filePath); | ||
|
||
expect(result).to.equal(''); | ||
|
||
realpathSyncNativeStub.restore(); | ||
}); | ||
|
||
it('should validate the correct path is returned when running on Windows', async () => { | ||
const filePath = 'c:\\Users\\User Name\\foo.cls'; | ||
const realpathSyncFilePath = 'C:\\Users\\User Name\\foo.cls'; | ||
const realpathSyncNativeStub = sinon | ||
.stub(fs.realpathSync, 'native') | ||
.returns(realpathSyncFilePath); | ||
const processPlatformStub = sinon.stub(process, 'platform').value('win32'); | ||
|
||
const result = flushFilePath(filePath); | ||
|
||
expect(result).to.equal(filePath); | ||
|
||
processPlatformStub.restore(); | ||
realpathSyncNativeStub.restore(); | ||
}); | ||
}); | ||
|
||
describe('flushFilePaths', () => { | ||
it('should return the same paths that are passed in', async () => { | ||
const filePaths = ['file.js', 'file.js', 'file.js']; | ||
const realpathSyncNativeStub = sinon | ||
.stub(fs.realpathSync, 'native') | ||
.returns(filePaths[0]); | ||
|
||
const result = flushFilePaths(filePaths); | ||
|
||
expect(result).to.equal(filePaths); | ||
|
||
realpathSyncNativeStub.restore(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this restore() be better placed in an afterEach() block? Or do you prefer to keep it here inside each test case block? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the moment, there's only one test, so I think keeping it central to the test is better. If there were more tests, and that stub was used in every test (and the stubbed value never changed), then placing it in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hum... I can't tell from the formatting but the
should those not be consolidated? I typically lean towards following patterns in tests to make them easier to grok for both new and existing developers so I'd prefer we put the stub/restore in before/after each blocks. This could be at the top level describe if that makes sense or just in a describe wrapper for all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh..above I mean I couldn't tell if both describe blocks were wrapped in a single top level describe. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gbockus-sf maybe we should take this off line & discuss f2f. realpathSyncNativeStub can't be hoisted or shared, b/c the mocked value that's returned changes from test to test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jeffb-sfdc Sure I'll ping ya in slack There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wrapped everything in a |
||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,17 +16,12 @@ import { | |
Command, | ||
CommandBuilder | ||
} from '@salesforce/salesforcedx-utils-vscode/out/src/cli'; | ||
import { | ||
notificationService | ||
} from '@salesforce/salesforcedx-utils-vscode/out/src/commands'; | ||
import { notificationService } from '@salesforce/salesforcedx-utils-vscode/out/src/commands'; | ||
import { flushFilePath } from '@salesforce/salesforcedx-utils-vscode/out/src/helpers'; | ||
import * as vscode from 'vscode'; | ||
import { nls } from '../messages'; | ||
import { | ||
testOutlineProvider | ||
} from '../views/testOutlineProvider'; | ||
import { | ||
forceAnonApexDebug | ||
} from './forceAnonApexExecute'; | ||
import { testOutlineProvider } from '../views/testOutlineProvider'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. more auto-formatting changes... |
||
import { forceAnonApexDebug } from './forceAnonApexExecute'; | ||
|
||
export async function forceLaunchApexReplayDebuggerWithCurrentFile() { | ||
const editor = vscode.window.activeTextEditor; | ||
|
@@ -75,21 +70,26 @@ function isAnonymousApexFile(sourceUri: vscode.Uri): boolean { | |
} | ||
|
||
async function launchReplayDebuggerLogFile(sourceUri: vscode.Uri) { | ||
await vscode.commands.executeCommand( | ||
'sfdx.launch.replay.debugger.logfile', | ||
{ | ||
fsPath: sourceUri.fsPath | ||
} | ||
); | ||
await vscode.commands.executeCommand('sfdx.launch.replay.debugger.logfile', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. more auto-formatting changes... |
||
fsPath: sourceUri.fsPath | ||
}); | ||
} | ||
|
||
async function getApexTestClassName(sourceUri: vscode.Uri): Promise<string | undefined> { | ||
async function getApexTestClassName( | ||
sourceUri: vscode.Uri | ||
): Promise<string | undefined> { | ||
if (!sourceUri) { | ||
return undefined; | ||
} | ||
|
||
await testOutlineProvider.refresh(); | ||
const testClassName = testOutlineProvider.getTestClassName(sourceUri); | ||
let testClassName = testOutlineProvider.getTestClassName(sourceUri); | ||
// This is a little bizarre. Intellisense is reporting that getTestClassName() returns a string, | ||
// but it actually it returns string | undefined. Well, regardless, since flushFilePath() takes | ||
// a string (and guards against empty strings) using the Non-null assertion operator | ||
// (https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-0.html#non-null-assertion-operator) | ||
// fixes the issue. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏽 |
||
testClassName = flushFilePath(testClassName!); | ||
|
||
return testClassName; | ||
} | ||
|
@@ -105,17 +105,16 @@ async function launchAnonymousApexReplayDebugger() { | |
|
||
async function launchApexReplayDebugger(apexTestClassName: string) { | ||
// Launch using QuickLaunch (the same way the "Debug All Tests" code lens runs) | ||
await vscode.commands.executeCommand( | ||
'sfdx.force.test.view.debugTests', | ||
{ | ||
name: apexTestClassName | ||
} | ||
); | ||
await vscode.commands.executeCommand('sfdx.force.test.view.debugTests', { | ||
name: apexTestClassName | ||
}); | ||
} | ||
|
||
export class ForceAnonApexLaunchReplayDebuggerExecutor extends SfdxCommandletExecutor<{}> { | ||
public build(): Command { | ||
return new CommandBuilder(nls.localize('force_launch_apex_replay_debugger_with_selected_file')) | ||
return new CommandBuilder( | ||
nls.localize('force_launch_apex_replay_debugger_with_selected_file') | ||
) | ||
.withLogName('force_launch_apex_replay_debugger_with_selected_file') | ||
.build(); | ||
} | ||
|
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.
👍🏽
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.
stupid questions b/c windows. The default is C:\ but can a user have a drive named a lower case letter? So would d:\ be valid vs D:? There may be a corner case here were a drive is actually a lower case letter (a quick google told me this does happen in some instances, like when using a usb drive). We probably need a follow on ticket here to address the corner case and having the ability to ask the filesystem the valid path.
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.
From what I can tell, what's displayed is always uppercase (
D:\
) but what the APIs/SDK uses is always lowercase (d:\
). I tested this on a VM running Windows, but it's VERY slow, so I'd like a more thorough test on a real Windows machine b/f we merge.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.
(also, if I remember correctly, Windows is case-insensitive, so
d:\
is the same asD:\
. When one performs aFileExists("c:\\foo\\bar.txt")
, it's the same asFileExists("C:\\FOO\\BAR.TXT")
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'm full of dumb questions. With the case insensitivity of windows why are we required to make the above change? It's been too long I don't remember what the 'down stream' changes are that result in the issue.
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 a really good question, and I wondered about that as well, but unfortunately, I don't know why the casing is failing. When I was debugging this, I noticed that the path was "c:\bla\bla\bla.cls" but fs.realpathSync.native() was returning "C:\bla\bla\bla.cls", and when I changed the case for the first char (the drive letter), our actions (deploy, delete, etc...) worked again.