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

Adding Testing Sidebar for ApexClasses #542

Closed
wants to merge 4 commits into from

Conversation

jkalloor0701
Copy link

What does this PR do?

This PR adds a sidebar that allows you to view, run, and interact with apex tests in your project.

What issues does this PR fix or reference?

PR fixes the lack of visibility in terms of running tests and viewing their errors.

@salesforce-cla
Copy link

salesforce-cla bot commented Aug 1, 2018

Thanks for the contribution! Unfortunately we can't verify the commit author(s): Justin Kalloor <j***@j***.i***.s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce.com Contributor License Agreement and this Pull Request will be revalidated.

@jkalloor0701
Copy link
Author

@vazexqi @allileong @lcampos

.appveyor.yml Outdated
@@ -54,6 +54,13 @@ test_script:
# Upload results for AppVeyor test tab
"Uploading Test Results:`n"
(New-Object 'System.Net.WebClient').UploadFile("https://ci.appveyor.com/api/testresults/junit/$($env:APPVEYOR_JOB_ID)" , (Resolve-Path .\junit-aggregate.xml) )
[xml]$results = Get-Content junit-aggregate.xml
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this. Don't change any of our build files.

@@ -14,5 +14,14 @@
"editor.tabSize": 2,
"editor.formatOnSave": true,
"prettier.singleQuote": true,
"rewrap.wrappingColumn": 80
"rewrap.wrappingColumn": 80,
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this.

package.json Outdated
@@ -47,6 +47,11 @@
"lerna exec --bail=false --ignore salesforcedx-vscode -- snyk test --severity-threshold=medium --show-vulnerable-paths=false",
"synk:monitor":
"lerna exec --bail=false --ignore salesforcedx-vscode -- snyk monitor --severity-threshold=medium --show-vulnerable-paths=false --org=vazexqi",
"start-webview": "npm run start --prefix=packages/salesforcedx-webview-ui",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to rebase onto the latest version from develop.

"view/title": [
{
"command": "sfdx.force.test.view.run",
"when": "view == sfdx.force.test.view",
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to come up with an analysis of whether we can support multiple test panels since that will inform us on how to continue.

package.json Outdated
@@ -47,6 +47,11 @@
"lerna exec --bail=false --ignore salesforcedx-vscode -- snyk test --severity-threshold=medium --show-vulnerable-paths=false",
"synk:monitor":
"lerna exec --bail=false --ignore salesforcedx-vscode -- snyk monitor --severity-threshold=medium --show-vulnerable-paths=false --org=vazexqi",
"start-webview": "npm run start --prefix=packages/salesforcedx-webview-ui",
Copy link
Contributor

Choose a reason for hiding this comment

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

These scripts were previously removed, any reason you need them?

"command": "sfdx.force.test.view.run",
"title": "Run Tests",
"icon": {
"light": "resources/light/play-button.svg",
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did we get the resources from? Are they OK from a copyright/IP perspective?

Copy link
Author

@jkalloor0701 jkalloor0701 Aug 1, 2018

Choose a reason for hiding this comment

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

We got them from mocha sidebar repo. @ntotten said that there is no problem with using them

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you send the link to that resource so I can look at the license and copyright?
This might seem pedantic but we need to be very clear on this to avoid legal issues.

Copy link
Author

Choose a reason for hiding this comment

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

@@ -1,6 +1,6 @@
#!/bin/bash

JORJE_LOCATION=/Users/nchen/Development/IdeaProjects/apex-jorje
JORJE_LOCATION=/Users/jkalloor/apex-jorje
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this. Or not check this file in.

@@ -12,23 +12,84 @@ import {
DEBUGGER_LINE_BREAKPOINTS
} from './constants';
import * as languageServer from './languageServer';
import { ApexTestOutlineProvider } from './views/testOutline';

export type ApexTestRequestInfo = {
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: Need to reconcile this with the changes that are happening on the jorje side.

apexClasses
);

const testViewItems = new Array<vscode.Disposable>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you refactor the following parts about running test into its own method to make it cleaner? The activate method is getting too long.

Copy link
Contributor

Choose a reason for hiding this comment

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

Look at how commands are registered on activate for salesforcedx-vscode-apex-replay-debugger and salesforcedx-vscode-core extensions.

@@ -1,6 +1,6 @@
#!/bin/bash

JORJE_LOCATION=/Users/nchen/Development/IdeaProjects/apex-jorje
JORJE_LOCATION=/Users/jkalloor/apex-jorje
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not check in changes to this file

export async function getApexTests(): Promise<{}> {
let response = {};
if (languageClient) {
response = await languageClient.sendRequest('apextest/getTestMethods');
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: Need to reconcile this with the changes that are happening on the jorje side.

@@ -41,6 +102,14 @@ async function getLineBreakpointInfo(): Promise<{}> {
return Promise.resolve(response);
}

export async function getApexTests(): Promise<{}> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a more specific type rather than {}?

@@ -24,6 +24,7 @@ export const messages = {
source_missing_text: '%s points to a missing folder',
java_runtime_missing_text:
'Java runtime could not be located. Set one using the salesforcedx-vscode-apex.java.home VS Code setting.',
force_apex_test_run_codeAction_description_text: 'Run Apex test(s)',
Copy link
Contributor

Choose a reason for hiding this comment

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

@ruthemmanuelle - Text review?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jkalloor0701 - Where will this message be displayed? Our style guidelines prohibit using (s) to show plurals. Can we just say Run Apex tests? Or, depending on the location where we show this, we might want to say Run Apex Tests (with all words capitalized).

Copy link
Author

Choose a reason for hiding this comment

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

screen shot 2018-08-02 at 12 57 11 pm

This is shown as a loading bar when the process is run. I can definitely change it to not have the (s), but I did grab this from other run test commands in the extension (just a heads up).

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Yeah, I think we talked about this decided that following the style guideline for the other run test commands would cause confusion. I think this is OK how it is. Thanks for the screenshot.

@@ -0,0 +1,69 @@
const FILE_0 = `public class file0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ignoring this file for now until we get the design/architecture nailed down.

"compilerOptions": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this just a formatting or did we change anything here?

Copy link
Author

Choose a reason for hiding this comment

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

I added "dom" to the "lib" option. There was an error as DOM types were not supported and this was a fix

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. And this is formatted using prettier?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I ran node scripts/reformatwithprettier.js so it should be formatted with that

@@ -48,4 +48,4 @@
"activationEvents": [
"*"
]
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this file.

@@ -0,0 +1,666 @@
import * as vscode from 'vscode';
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a copyright header. See the other files.

const notificationService = sfdxCoreExports.notificationService;
const taskViewService = sfdxCoreExports.taskViewService;

const LIGHT_BLUE_BUTTON = ospath.join(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would pull all these LIGHT_*_BUTTONS into a different file to keep this file cleaner.

export type TestSummary = {
outcome: string;
testsRan: number;
passing: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick. Why is this called passing instead of passed? Similary, why is it called failing instead of failed?

Copy link
Author

Choose a reason for hiding this comment

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

That's just what it is called in the actual json output, so I left that as that

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Maybe we should move all these files that are just mapping the JSON into a separate file called TestDataAccessObjects.ts so that it's clearer that this is just mapping to the JSON since you have no control over that.

The JSON is "stable" right? And won't change?

Copy link
Author

Choose a reason for hiding this comment

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

It shouldn't change unless there is a change to the actual sfdx force:apex cli commands themselves

private static testStrings: Set<string> = new Set<string>();

constructor(
private path: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use this style of declaring fields. Just declare the fields normally.

return this.head;
}

public getChildren(element: Test): Thenable<Test[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Must we use Thenable? Can we use a combination of async/await? Shouldn't mix different styles of programming paradigms if possible.

return Promise.resolve(this.head.children);
} else {
const emptyArray = new Array<ApexTest>();
emptyArray.push(new ApexTest('No tests in folder', null, 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

@jkalloor0701 - All messages should be externalized.
@ruthemmanuelle - Comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jkalloor0701 - How will this message be displayed, and when do users encounter it? That will affect the feedback I have on it. And yes, it should be externalized.

Copy link
Author

Choose a reason for hiding this comment

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

screen shot 2018-08-02 at 12 49 48 pm

The message is shown in the sidebar itself, and the user encounters it when there are no tests in the project

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @jkalloor0701 . Would it make sense to give the user a bit more info here? We could add some language like this, which I adapted from a similar situation we encountered in the past.

No Tests Found
Your project doesn't contain any Apex test methods. To run Apex tests, open a project that contains methods with @istest annotations or the testMethod keyword.

Copy link
Author

Choose a reason for hiding this comment

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

I could add that. It might be a little long for that small amount of space. Can I have that be the description when you hover over?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, having the sentence be hover text works. In that case, please change the label to "No Apex Tests Found".

private eventsEmitter = new events.EventEmitter();

private apexTestMap: Map<string, Test> = new Map<string, Test>();
private head: Test | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Pick a better name than head.

Copy link
Author

@jkalloor0701 jkalloor0701 Aug 2, 2018

Choose a reason for hiding this comment

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

Well, it's the object that contains all the Test Classes and tests, so container?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the rootNode for all child tree items (tests, test classes, etc)?
Maybe rootNode or treeRoot?

};

export class ApexTestOutlineProvider implements vscode.TreeDataProvider<Test> {
private _onDidChangeTreeData: vscode.EventEmitter<
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use the _convention for private methods/fields.

apexClasses = (await getApexTests()) as ApexTestRequestInfo[];
}

const apexClassesDocs = await vscode.workspace.findFiles('**/*.cls');
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for? Also, this is not correct. You cannot just find all the .cls files in a project. You need to look into sfdx-project.json and only look into parts that are in the packageDirectories. That is what the Apex LSP does.

So, if you do it this way, there is a possibility that the results will be different.

Copy link
Author

Choose a reason for hiding this comment

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

I know sometimes when I used debug to run an extension, I would run into problems with using lsp. I am using this as a backup. If that is not necessary I can get rid of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you elaborate more on the issue with the LSP?
I think it would be nice to not have to use this manual approach.

Copy link
Author

Choose a reason for hiding this comment

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

When I first launch the extension, isLanguageClientReady() will return false (probably has to do with the fact that activate is async and and createLanuageServer is called in activate). When that happens, I use the manual parsing approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

The LSP does take some time to start up. And until it starts up, you would not be able to get the results yet. Are you pre-loading the test view eagerly on startup even if the view is not displayed? Could we do this lazily? How do the other test runners (maybe take a look at Mocha etc ) do it – eagerly or lazily?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, what I could do is wait for the LSP to start up and then refresh the view. Also, manual parsing does not seem to work at all for a large workspace such as steelbrick, so I am inclined to get rid of it.

} else {
this.getAllApexTests(this.path);
if (!(this.head && this.head.children.length > 0)) {
this.head = new ApexTest('No tests in folder', null, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

@jkalloor0701 - All messages should be externalized.
@ruthemmanuelle - Comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jkalloor0701 - I have the same questions about this as about the message that's currently on line 176.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jkalloor0701 - Is this the same situation as the one above?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it is

if (isLanguageClientReady()) {
this.apexTestInfo = (await getApexTests()) as ApexTestRequestInfo[];
}
this.apexClasses = await vscode.workspace.findFiles('**/*.cls');
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong. See previous comment on the explanation why.

"activationEvents": ["workspaceContains:sfdx-project.json"],
"activationEvents": [
"workspaceContains:sfdx-project.json",
"onCommand:sfdx.force.test.view.showError",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to add this new activation events if the extension would already get activated if the project is in sfdx shape ?

return this.head;
}

private addTests(fileContent: string, apexTestGroup: ApexTestGroup): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this? It seems like this is very error-prone since it's not actually parsing but matching regex. You could be confused if you have a commented portion of code like

// @IsTest

or

/**
* @IsTest
**/

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I didn't want to attempt to refine in case we didn't need it, but I outline why we need it as of now above.


if (test.file) {
vscode.window.showTextDocument(test.file).then(() => {
this.eventsEmitter.emit('Show Highlight', position, isRow);
Copy link
Contributor

Choose a reason for hiding this comment

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

@jkalloor0701 - All messages should be externalized.
@ruthemmanuelle - Comments?

Copy link
Contributor

@ruthemmanuelle ruthemmanuelle Aug 2, 2018

Choose a reason for hiding this comment

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

@jkalloor0701 - Like other messages, to provide feedback I'd like to know:

  1. When users will encounter this message.
  2. Where and how the message will be displayed.

And, yes, all messages should be in a messages file. That way we'll be able to translate them if we localize the extension pack, and it's easier for me to keep an eye on any changes to the messages that people make in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jkalloor0701 - Just making sure you saw this comment, since you already responded to my other questions.

Copy link
Author

Choose a reason for hiding this comment

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

Oh whoops, forgot to respond to this one. This is actually not a message but the event emitter launches a different function to run. @ruthemmanuelle

}
}

private deleteFolderRecursive(path: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for? What are you deleting?

Copy link
Author

Choose a reason for hiding this comment

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

I output results to a temporary output directory, and then I go and delete it. This is so that we can still output the human output to the console. @cwallsfdc mentioned that this would be a good option to use

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no utility that we can use to delete this short of writing our own recursive delete? Maybe you can search for shelljs and see how we use it there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if you are using the OS temp folders, aren't those deleted automatically for us?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not using OS temp folders, would that work on Windows?

Copy link
Author

Choose a reason for hiding this comment

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

shell.rm('-rf', path) is not working. I could use the /tmp OS temp folder for Mac and Linux. For windows, the temp folder is stored in the TEMP environment variable. Not sure if that is a good approach either

Copy link
Contributor

Choose a reason for hiding this comment

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

All right, let's tackle the issue holistically here since I might be missing something.

Intent

Purpose (please confirm/refute): we need a way to store temporary results so that we can still output human results (since JSON is not human readable results, I assume).

Questions:

  1. How bad is the JSON results from a human-readable perspective?
  2. How come we didn't choose to use the TAP format from the test runner? That one seems to be able to stream results through and is also pretty human-readable.

Implementation

If we do need to store the results somewhere temporarily, could we use some of the features of the OS to simplify our implementation? Though, some posts like this seems to suggest that Windows doesn't clean out the temp folders automatically.

  1. Does https://nodejs.org/docs/latest-v7.x/api/os.html#os_os_tmpdir not work? Note, I am using Node 7's doc because that is the version of Node that VS Code ships with as of August 2018.
  2. "shell.rm('-rf', path) is not working" <-- what is the error that you are seeing?

Copy link
Author

Choose a reason for hiding this comment

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

Intent:

  1. The JSON results are definitely not very readable, and especially if I want to later allow any invocation of these tests to update the test view (such as the code lens button).
  2. I followed the example of the other run test examples in the extension.

Implementation:

  1. Yes, that works. I will have to do some tests on Windows to see if that folder gets cleaned automatically.

  2. There is no elaboration on the error, even if I turn on the developer tools in the extension host, it just does not remove the folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

"I want to later allow any invocation of these tests to update the test view (such as the code lens button)." <-- this is a good point. Does the code lens button fire off any events now that the test view can list for?

Copy link
Author

Choose a reason for hiding this comment

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

Well, it uses the forceApexTestRunCodeActionExecutor, so I was going to utilize that

const jsonSummary = this.getJSONFileOutput(folderName);
this.UpdateTestsFromJSON(jsonSummary);
this._onDidChangeTreeData.fire();
this.eventsEmitter.emit('Delete Folder', fullFolderName);
Copy link
Contributor

Choose a reason for hiding this comment

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

@jkalloor0701 - All messages should be externalized.
@ruthemmanuelle - Comments?

Copy link
Author

Choose a reason for hiding this comment

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

this.eventsEmitter.emit is not a message, it launches the deleteFolder function.

Copy link
Contributor

Choose a reason for hiding this comment

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

✅ - OK nothing to do here then.

});
}

private summarize(summary: TestSummary, group: ApexTestGroup): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this into its class in a different file to make it more easily testable.
In general I'm not a fan of just using string concatenation.
Is there a different approach that we can use?

public name: string;

constructor(
public label: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use this method of declaring fields.

public passing: number = 0;

constructor(
public label: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use this style of declaring fields.

public passed = false;

constructor(
public label: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use this style of declaring fields.

Copy link
Contributor

@vazexqi vazexqi left a comment

Choose a reason for hiding this comment

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

I suggest prioritizing making the Apex LSP changes first since that changes how the protocol communicates.

}

// Test View
const rootPath = vscode.workspace.rootPath || '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Use vscode.workspace.workspaceFolders[0] instead of rootPath since it's deprecated (https://github.com/Microsoft/vscode/wiki/Extension-Authoring:-Adopting-Multi-Root-Workspace-APIs)

@vazexqi
Copy link
Contributor

vazexqi commented Aug 1, 2018

@jkalloor0701 - Have you tested this on Windows?

"activitybar": [
{
"id": "test",
"title": "Test Viewer",
Copy link
Contributor

Choose a reason for hiding this comment

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

We keep labels in our package.json files in corresponding package.nls.json files so that we can more easily apply localization. You can look at line 140 of this file as an example. Could you please extract the user facing strings in this file to the package.nls.json file?

channelService.showChannelOutput();
}

await ProgressNotification.show(execution, cancellationTokenSource);
Copy link
Contributor

@allileong allileong Aug 1, 2018

Choose a reason for hiding this comment

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

In a recent change #536 , I removed the await on ProgressNotification.show and it should be removed here as well. After removing the await, the async should also be removed from line 640.

Copy link
Contributor

@ruthemmanuelle ruthemmanuelle left a comment

Choose a reason for hiding this comment

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

@jkalloor0701 - To summarize the text-related discussions:

  1. Please make the changes we discussed in the comment thread on line 176 of packages/salesforcedx-vscode-apex/src/views/testOutline.ts, which should also apply to line 188.
  2. Please move all user-facing text to packages/salesforcedx-vscode-apex/src/messages/i18n.ts.

Other than that, the text looks good.

@jkalloor0701
Copy link
Author

Whoops, some reverts didn't work. Let me just change those

@@ -24,6 +24,11 @@ export const messages = {
source_missing_text: '%s points to a missing folder',
java_runtime_missing_text:
'Java runtime could not be located. Set one using the salesforcedx-vscode-apex.java.home VS Code setting.',
force_apex_test_run_codeAction_description_text: 'Run Apex test(s)',
force_test_view_loading_message: 'Loading Apex Tests ...',
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually use sentence-case capitalization for progress messages: "Loading Apex tests..."

@vazexqi
Copy link
Contributor

vazexqi commented Aug 10, 2018

Closed in favor of #552

@vazexqi vazexqi closed this Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants