Fixed bug in getTestClassName(), added try/catch to forceLaunchApexReplayDebuggerWithCurrentFile()

[jeffb-sfdc]

What does this PR do?

This PR address a few issues that were recently discovered with the SFDX: Launch Apex Replay Debugger with Current File command.

What issues does this PR fix or reference?

#@W-12508503@

Functionality Before

We discovered two issue:
First, when the user ran the SFDX: Launch Apex Replay Debugger with Current File command, it didn't work. Meaning, to the user the Apex debugger never started.

Second, even though the operation failed, the user was never told anything, are weren't told it had failed.

Functionality After

• A try/catch block was added to forceLaunchApexReplayDebuggerWithCurrentFile(). If an exception occurs, the exception is caught and we now display a notification to the user and log the exception into the Output panel.
• When getApexTestClassName() is called, we now no longer call flushFilePath(). Instead, getTestClassName() converts the file path string and the paths stored in the Map (testIndex) to lower case and compares the two strings.

Notes for reviewers and QE

• When running SFDX: Launch Apex Replay Debugger with Current File, this was only an issue with debugging Apex class files (not debugging anonymous Apex files, or Apex log files), though it wold be good to have all three paths/files types verified.
• To repro the handling of the exception and displaying the notification & output, this will be hard to naturally reproduce. I suggest running code from this branch and modify forceLaunchApexReplayDebuggerWithCurrentFile() to throw an exception, and then verify the results in the UI.
• This should be QA'd on windows as well.
• Unit tests will be coming in a separate PR. I'm going to perform some refactoring of files containing multiple classes in them into separate files, so there will be a few extra files touched in for the unit test work, and adding those changes here in this PR will (IMHO) cloud the changes I made to fix this issue.
• Other places in our code where flushFilePath() is called are:

• forceSourceDelete
• LibraryPathsGatherer, which is used in forceSourceDeploySourcePath and forceSourceDeploySourcePath

I verified that all three work, but the QE should also test/verify each of these three features/areas as well on both mac and Windows

[jeffb-sfdc]

Lines 26-60 in the new version are lines 25-59 in the old version. No code changes were made here, other than wrapping in a try block and indenting.

[jeffb-sfdc]

In the catch block, a notification was added and logging to the Output view was added. A new string was added and the verbiage was created by Sonal.

[jeffb-sfdc]

We now no longer use flushFilePath() here. Instead, getTestClassName() converts the paths to lower case for the comparison.

I believe Windows isn't able to have multiple files with different casing in the same location (eg fooBar.txt and foobar.txt)

Linux (I think) does support this and one can have files with different casings at the same time (in the same location), but I think the chances of someone having both myapexclass.cls and MyApexClass.cls in the same location are pretty slim.

And with Mac OSX, even though its file system is based in Linux, its UI (the Finder) prevents one from naming files with different casing in the same location.

[gbockus-sf]

So what we're talking about here is a case-sensitive file system. All OS's have the ability to have case sensitivity turned on for the file system (it's more complicated then that for windows in that it's a more recently supported thing and can be enabled on the folder level :cringe: source. It's been a while but last I recall salesforce doesn't enforce case sensitivity in the org, so I think we're clear to assume that there will never be two files with the same name but different cases.

I did a quick google to see if I could find the official SF doc on this and found a few external hits but nothing official. General agreement seems to be that apex class name are case insensitive so we're good to go. I also verified that if I tried to create a new class with the same name but different casing it errored 👍

[gbockus-sf]

I'm not sure this comment is valuable. This is only true on mac, and only if the sf project is under the users home directory (it could be anywhere on the filesystem) It could be misleading to our future selves reading it.

[randi274]

I had the opposite reaction of "oh how useful!" 😄 But I get your point here.

[jeffb-sfdc]

When I first discovered the bug and I was stepping through the code, it was not obvious what the issue was, which was, when "fs.realpathSync.native" is called, it's sometimes called with a path of "file://Users/..." and sometimes with a. path of /Users/..., and when passed a string with "file:" in it, it would thrown an exception.

Knowing what I do now, yea, it's obvious, but when I was initially looking into it, it wasn't obvious.

OK, so why use paths with "file:" in them at all? We have code (like what's in testOutlineProvider.ts) which relies on it. I'm not aware of the historical reasons why this is, but would prefer to not change additional components/areas with this PR.

Since the path can be two different formats, I think it's helpful to comment what the expected file path is in these two areas.

[gbockus-sf]

That's cool. In an ideal world we should create types and remove the mental type checking of strings ref. In a less ideal world we should validate the filePath parameter matches what we expect and throw an error if not correct.

The part I take issue with here is /Users/{user-name}/. It's inaccurate and could lead devs down a bad assumption path about what to expect from the input parameter.

[jeffb-sfdc]

"The part I take issue with here is /Users/{user-name}/..."

I agree. I still need to run this on Windows and verify the fix works. (I only have a VM for this, and it's dog slow and I've been swamped this week and haven't gotten to this yet). I'll post an update here with what I find. I know the format will be different (eg /Users/{user-name}/... vs C:/Users/{user-name}/... so I will probably document both expected Mac and Windows paths in both locations.

[jeffb-sfdc]

Randi ran the branch on her Windows machine this morning it ran w/o any issues (the file paths worked). I added comments on what the expected path for Mac OSX and Windows are expected.

[gbockus-sf]

I know there are existing patterns in place, but this feels dangerous in that JSON.stringify is a good way to get another error thrown that won't be caught (and it'll just be nonsense in the console out if displayed).

The right thing to do here would be to add a method to our utils package to process errors and give us back a string to display to the user. Some quick googling led me to this which in it's final form is pretty bulletproof, but I get we don't want to blow this up so maybe the solution in this SO makes more sense with a WI to address error handling across the board.

[klewis-sfdc]

Does error.message need to be displayed in the channel at all? Since it is already outputting the generic "Unknown error when launching..." message, maybe the specific error here could be output as a console.warn() or console.error().

Either way, I do agree that error.message needs to be checked before the call to JSON.stringify(). I like this structure on the bottom of the SO page that Gordon linked to: https://stackoverflow.com/a/68240891.

[jeffb-sfdc]

@klewis-sfdc , yes, it should be displayed, otherwise we won't have knowledge of what happened or how to go about fixing the issue.

re. if (err instanceof Error): there's an edge case which this misses, which is if the Message is empty, the error reported is an empty string, an this instead handles this case:

const message = (error && error.message)
      ? error.message
      : JSON.stringify(error);

[gbockus-sf]

@jeffb-sfdc do you agree we need to wrap JSON.stringify in a try/catch?

[gbockus-sf]

So the root of the issue here was flushFilePath throwing an error correct? I was trying to wrap my head around this change and didn't really grok the root problem.

One big downside to the change is we are switching from a single lookup in a map to iteration over all tests that exist to see if any match (with an extra operation to lowercase the path for each test). Granted for small use cases this probably doesn't matter, but we support larger orgs that have hunderds of not thousands of tests so the cost here is potentially high.

An alternate approach would be to move the flushFilePath call to before the getTestClassName call

  const flushedPath =  fileUtils.flushFilePath(sourceUri);
  const testClassName = testOutlineProvider.getTestClassName(flushedPath);

To verify this I wanted to see what was happening with flushFilePath. It appears there have been issues with the version of node used by VSCode (which explains why we didn't see this originally but it's showing up now) and the usage of realpathSync.native ref.

[gbockus-sf]

We may want to add a try/catch to flushFilePath. I did a quick search and it appears that a pretty typical approach: example

Thoughts?

[klewis-sfdc]

Agree with what Gordon has said so far here. Re: iterating over all test files within getTestClassName(), what do you think about lowercasing the testIndex entries when it is built in createTestIndex() and then lowercase the uri filename when checking the index this.testIndex.get(uri.toString().toLowerCase());?

[jeffb-sfdc]

@gbockus-sf I don't like that approach. For one, I think handling an exception at that level is too low level and not the appropriate place.

Second, the function is handling the exception, and is also eating an exception and not letting callers handle it.

Here is the suggested code with walk through and comments:

export default function tryRealpath(path: string): string {
  try {
    path = realpathSync.native(path);
  } catch (error: any) {
    if (error.code !== 'ENOENT') {
      throw error;
      // if an exception happens and error.code !== 'ENOENT', we just re-throw the exception,
      // so not much value here
    }
    // else..?
    // the exception is eaten...
  }

  return path;
  // ... and the path is "successfully" return...
  // but this introduces a bug - the error is now eaten.
}

[gbockus-sf]

yeah I'm not a fan of unhandled exceptions myself...but I do trust some of the larger projects have flushed out the details around the native method and I'd rather lean on their experience then learn all the quirks of that method ourselves. I wish I could just share the link to the real work examples I'm seeing in vscode but alas it doesn't work that way. I see this by hovering over the native method and clicking the 'See Real World example from Github' link.

Typescript
Jest
eslint limits it to specific os
opensumi (some framework to build vscode extenions 🤷 )

There's a few others and not all swallow the exception but those that I looked at appears to be filtering by os and other method prior to calling the native method.

I'd totally be down to have a catch that posts a custom event to our analytics with the path so we could try and track down when the native method throws. (we could also add a console.log so we could easily identify it's happening with the end user).

[gbockus-sf]

See comments. Biggest concern is perf impact of iterating over all test files in this code flow. Happy to chat here or in slack 👍

[jeffb-sfdc]

Randi ran this branch on her Windows machine this morning. The code ran w/o any additional changes needed, and I added a comment here of what the format of filePath is here on Windows.

[gbockus-sf]

I probably wasn't clear in my original comment. The issue with this comment is the path isn't guaranteed to be in the users home space on either windows or mac. /Users/{user-name}/ and \\Users\\{user-name}\\ aren't always present. Is the point of this comment to communicate that the filePath string will not include the file:/ scheme? If so I could see going with something like

// The filePath should be an fs filepath and not a VSCode URI string. No scheme (file:/) should be present.
// Examples: 
// "/Users/{user-name}/{path-to-apex-file.cls}" on Mac OSX
// "c:\\Users\\{user-name}\\{path-to-apex-file.cls}" on Windows

[jeffb-sfdc]

Randi ran this branch on her Windows machine this morning. The code ran w/o any additional changes needed, and I added a comment here of what the format of filePath is here on Windows.

[jeffb-sfdc]

Randi ran this branch on her Windows machine this morning. The code ran w/o any additional changes needed, and I added a comment here of what the format of filePath is here on Windows.

[gbockus-sf]

closing for now