Skip to content

Lowercase drive letter on localhost path mappings on Windows#6023

Merged
kimadeline merged 5 commits intomicrosoft:masterfrom
kimadeline:5362-path-mapping
Jun 14, 2019
Merged

Lowercase drive letter on localhost path mappings on Windows#6023
kimadeline merged 5 commits intomicrosoft:masterfrom
kimadeline:5362-path-mapping

Conversation

@kimadeline
Copy link

@kimadeline kimadeline commented Jun 13, 2019

For #5362

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Appropriate comments and documentation strings in the code
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated
  • Test plan is updated as appropriate
  • package-lock.json has been regenerated by running npm install (if dependencies have changed)
  • The wiki is updated with any design decisions/details.

@kimadeline kimadeline changed the title WIP - 5362 path mapping on Windows WIP - Lowercase drive letter on localhost path mappings on Windows Jun 13, 2019
@kimadeline kimadeline changed the title WIP - Lowercase drive letter on localhost path mappings on Windows Lowercase drive letter on localhost path mappings on Windows Jun 13, 2019
@kimadeline kimadeline marked this pull request as ready for review June 13, 2019 22:46
Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

Please add the additional tests before merging.

@DonJayamanne
Copy link

@kimadeline
Sorry, thought I saw something that was incorrect. Its all good.
Just one suggestion, please try avoiding the use of if... conditions to skip tests.
Assume we had 3-4 tests or more, what happens is, we have a large if... block and code gets difficult to read E.g. i scroll down and read the test and fail to realize that its only being tested under a specific scenario.

You can instead use this.skip() inside tests to skip tests.

@kimadeline
Copy link
Author

Good point, I'll move the if (windows) condition inside the test and do something like:

test(`Ensure drive letter is lower cased for local path mappings on Windows when host is '${host}'`, async () => {
   if (os.name === 'Windows') {
      this.skip();
   }

   // test code
});

@DonJayamanne
Copy link

DonJayamanne commented Jun 14, 2019

Remember:

  • Don't use arrow function, else you lose scope (this is different)
  • Use return (people forget this).
test(`Ensure drive letter is lower cased for local path mappings on Windows when host is '${host}'`, async function (){
   if (os.name === 'Windows') {
      return this.skip();
   }

   // test code
});

@kimadeline kimadeline merged commit bbaf189 into microsoft:master Jun 14, 2019
@kimadeline kimadeline deleted the 5362-path-mapping branch June 14, 2019 18:21
@lock lock bot locked as resolved and limited conversation to collaborators Jul 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants