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

Allow rename of lwc component inside of __tests__ directory #4225

Merged
merged 18 commits into from
Jul 11, 2022

Conversation

dehru
Copy link
Contributor

@dehru dehru commented Jun 21, 2022

What does this PR do?

This PR adjusts the when clause for the Rename Component command so that it works in any file in a subdirectory of the default lwc or aura with the right file extensions. It also adds a simple utility function that will resolve the directory above the __tests__ directory so that the rest of the rename component functionality works as expected.

What issues does this PR fix or reference?

##4053, @W-11152868@

Functionality Before

rename-before.mov

Functionality After

lwc-component-rename.mov

@dehru dehru requested a review from a team as a code owner June 21, 2022 21:56
@dehru dehru requested a review from jeffb-sfdc June 21, 2022 21:56
@@ -307,7 +307,7 @@
},
{
"command": "sfdx.lightning.rename",
"when": "sfdx:project_opened && resource =~ /.*/(lwc|aura)/[^/]+(/[^/]+\\.(html|css|js|xml|svg|cmp|app|design|auradoc))?$/"
"when": "sfdx:project_opened && resource =~ /.*/(lwc|aura)/.*(/[^/]+\\.(html|css|js|xml|svg|cmp|app|design|auradoc))?$/"
Copy link
Contributor Author

@dehru dehru Jun 21, 2022

Choose a reason for hiding this comment

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

Basically, show the command to the user on anything inside the /lwc/ and /aura/ directories that have these file extensions. This includes the component subdirectory named __tests__ and files inside there.

If the user comes up with their own folder structure with a folder named lwc, this should still work to resolve the parent component directory.

@dehru dehru force-pushed the dehru/fix-rename-component-test branch from 7945fba to de82062 Compare June 22, 2022 19:36
return stats.isFile() ? path.dirname(sourceFsPath) : sourceFsPath;
let dirname = stats.isFile() ? path.dirname(sourceFsPath) : sourceFsPath;
if (dirname.endsWith(TEST_FOLDER)) {
dirname = getParentDirectoryOfTestFolder(dirname, path.sep);
Copy link
Contributor

Choose a reason for hiding this comment

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

since path.sep is coming from a module we're importing, I don't think it needs to be passed into getParentDirectoryOfTestFolder()

Copy link
Contributor Author

@dehru dehru Jun 24, 2022

Choose a reason for hiding this comment

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

This is where i got tripped up with Windows vs OSX in unit testing. How you suggested was my original implementation. I'd appreciate your thoughts on it.

If I use path.sep inside the function itself instead of passing it in, it works great at runtime. The path that is generated by the OS can be parsed using path.sep.

But during testing, i wanted to unit test that this function works on both platforms. if I want to pass in a string like this force-app/main/default/lwc/helloworld/__tests__ and verify that it get parsed and the __tests__ directory removed, the test only passes on my OSX operating system and fails on our Windows CI. Because on Windows the function under unit test is now looking for \ and the unit test fails.

I thought about generating the string in the unit test dynamically like force-app${path.sep}main${path.sep}default${path.sep}lwc${path.sep}helloworld${path.sep}__tests__. And maybe that's better, but it just seemed hard to read and might be hard to debug a failure.

So anyway, I opted to pass in the path.sep into the function, and this allowed me to unit test the function for both operating systems, and it's easy to read the unit test.

So options are:

  1. Leave it as is ( pass the separator into the function )
  2. Generate the path dynamically in the unit test and use path.sep inside the function.
  3. Use an if/else in the unit test. Like if ( path.sep === '/' ) {} else {} inside the it( statement of the unit test based on environment.

Copy link
Contributor

@jeffb-sfdc jeffb-sfdc Jun 28, 2022

Choose a reason for hiding this comment

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

I looked trough our codebase, but I don't see many places where we are creating platform-specific tests. I just ran through the exercise of, "what tests would I write if I had written getParentDirectoryOfTestFolder()"?

I started off with just the implementation:

export function getParentDirectoryOfTestFolder(sourceFsPath: string): string {
  const directories = sourceFsPath.split(path.sep);
  directories.splice(directories.length - 1);
  return directories.join(path.sep);
}

...and then thought about writing a test for it. This is what immediately came to mind:

it('validates getParentDirectoryOfTestFolder', () => {
    const folders = ['path', 'to', 'tests']; // mocked(fake) path
    const folderPath = folders.join(path.sep);

    const parentFolder = folders.slice(0, -1);
    const parentFolderPath = parentFolder.join(path.sep);

    const parentDirectory = getParentDirectoryOfTestFolder(folderPath);
    expect(parentDirectory).to.equal(parentFolderPath)
  });

With this test, it validates the logic & behavior inside the function, and leaves the implementation as a black box.

OK, so, thinking out loud here... I could then see one saying that my test is just mirroring the implementation of what's in getParentDirectoryOfTestFolder(), which I would say, that's OK, we just want to validate that it works.
But if that wan't acceptable, the next thing I came up with was a test like this:

it('validates getParentDirectoryOfTestFolder', () => {
    const folderPath = 'path|to|tests'.replace(/\|/g, path.sep);
    const parentFolderPath = 'path|to'.replace(/\|/g, path.sep);

    const parentDirectory = getParentDirectoryOfTestFolder(folderPath);
    expect(parentDirectory).to.equal(parentFolderPath)
  });

const parentFolderPath = parentFolder.join(path.sep);

const parentDirectory = getParentDirectoryOfTestFolder(folderPath);
expect(parentDirectory).to.equal(parentFolderPath);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeffb-sfdc - I ended up with your first suggestion. I think this is a good balance. Thanks for the input.

@klewis-sfdc
Copy link
Contributor

klewis-sfdc commented Jun 29, 2022

I can see the right-click option to rename the component when I am over a .json file within the component directory structure, but the command doesn't work if that file is within another directory (like accountMap/tests/data). What do you think about either hiding the option from the right click menu here, or updating so that the rename action works correctly?

2022-06-29_07-34-45.mp4

Another example would be files within the "/templates" directory (looking at errorPanel component in eBikes)

2022-06-29_07-58-18

@dehru
Copy link
Contributor Author

dehru commented Jun 30, 2022

@klewis-sfdc Thanks for pointing me to eBikes as a source of interesting folder structure inside the aura and lwc directories that I hadn't considered. This led me to refactor how we look up the component directory. Now we search the directory path until we find the last instance 'lwc' or 'aura', and then the component directory exists below that.

Here's a movie showing me exercise the bugs you found above.

rename-refactored.mov

@dehru
Copy link
Contributor Author

dehru commented Jun 30, 2022

NOTE, I'm seeing that same error from CI locally. sobjects-faux-generator is getting a 404 from npm trying to install @types for eslint.

@@ -639,6 +639,8 @@ export const messages = {
'Press Enter to confirm your input or Escape to cancel',
rename_component_warning:
'Warning: References to the old name will not be updated. Update manually and redeploy once all changes have been made.',
rename_component_error:
'Unable to rename component. Update manually and redeploy once all changes have been made.',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbudhirajadoc - I added a pop-tart message if there is ever an error running this command. I noticed that we also throw a generic failure message. So this might not be necessary. Let me know what you think.

The nasty details of the error message will display in the output channel.
Screen Shot 2022-06-30 at 2 31 33 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

@dehru I am not clear on what the action "Update manually and redeploy once all the changes have been made" means. What do they update? How do they redeploy?

Copy link
Contributor

@klewis-sfdc klewis-sfdc left a comment

Choose a reason for hiding this comment

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

Looks great @dehru! Thanks for the improvements!

  • [x ] SFDX: Rename Component command shows up in right click menu when under main files within tests directory under LWC or Aura component structure
  • [x ] SFDX: Rename Component command works to rename main files with same name as component under LWC or Aura component structure

@dehru dehru merged commit 3da9c39 into develop Jul 11, 2022
@dehru dehru deleted the dehru/fix-rename-component-test branch July 11, 2022 17:19
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.

5 participants