Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Add autoload for TestLanguage.psm1 TestHelpers.psm1#3456

Merged
TravisEz13 merged 7 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:tests-auto-ipmo-3iSazonov/PowerShell:tests-auto-ipmo-3Copy head branch name to clipboard
May 17, 2017
Merged

Add autoload for TestLanguage.psm1 TestHelpers.psm1#3456
TravisEz13 merged 7 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:tests-auto-ipmo-3iSazonov/PowerShell:tests-auto-ipmo-3Copy head branch name to clipboard

Conversation

@iSazonov

@iSazonov iSazonov commented Mar 30, 2017

Copy link
Copy Markdown
Collaborator

The test temporary modules moved to test\tools\Modules.
I did two commits to simplify the review:

  1. Commit for Test.Helpers.psm1 (was renamed to TestHelpers.psm1)
  2. Commit for LanguageTestSupport.psm1 (was renamed to TestLanguage.psm1)

@TravisEz13 TravisEz13 requested a review from JamesWTruher March 30, 2017 16:58
lzybkr
lzybkr reviewed Apr 4, 2017
View reviewed changes

@lzybkr lzybkr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2 small comments

  • you can delete most of the comments from the module manifest, they aren't useful
  • can you think of a better name for the module TestLanguage - it feels too similar to a hypothetical script or cmdlet. Maybe LanguageTestHelpers - and maybe TestHelpers should be CommonTestHelpers, or something like that.

@iSazonov

iSazonov commented Apr 4, 2017 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Can we add a "Module" suffix?

  • TestCommonHelpersModule
  • TestLanguageModule
  • TestHostCSModule
  • TestRemotingModule

@lzybkr

lzybkr commented Apr 4, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Mostly I'm just thinking out loud here, it just struct me as odd seeing verbs in module names. I was curious how common this was, and it's not uncommon:

$verbs = (Get-Verb).Verb
Find-Module | ? { $modName = $_.Name; $verbs | ? { $modName.StartsWith($_) } }

This gives my 97 modules, some of which sound more like scripts than modules, but it's hard to see without looking more closely.

At any rate, consider this just one opinion, nothing more.

@iSazonov

iSazonov commented Apr 4, 2017

Copy link
Copy Markdown
Collaborator Author

I thought about how to easily find these modules using Get-Module. A common prefix can help instead of Test verb:

  • HelpersCommon
  • HelpersLanguage
  • HelpersHostCS
  • HelpersRemoting

@iSazonov iSazonov force-pushed the tests-auto-ipmo-3 branch 2 times, most recently from e040758 to a138e0d Compare April 5, 2017 04:43
@iSazonov

iSazonov commented Apr 6, 2017

Copy link
Copy Markdown
Collaborator Author

@lzybkr @TravisEz13 In last commit I renamed modules (Removed approved verbs (Get-Verb) from module names).
Please continue the review.

@iSazonov

iSazonov commented Apr 13, 2017

Copy link
Copy Markdown
Collaborator Author

@lzybkr @TravisEz13 Do I need something else to do here?

@iSazonov iSazonov force-pushed the tests-auto-ipmo-3 branch from 5a3e8fd to e8d7619 Compare April 19, 2017 06:16
@iSazonov

iSazonov commented Apr 19, 2017

Copy link
Copy Markdown
Collaborator Author

@lzybkr @TravisEz13 Could you please review and merge? This blocks further work.

@iSazonov iSazonov mentioned this pull request Apr 19, 2017
Merged
2 tasks
TravisEz13
TravisEz13 reviewed Apr 23, 2017
View reviewed changes
Comment thread test/tools/Modules/HelpersCommon/HelpersCommon.psd1 Outdated

@TravisEz13 TravisEz13 Apr 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are still comments here

TravisEz13
TravisEz13 reviewed Apr 23, 2017
View reviewed changes
Comment thread test/tools/Modules/HelpersHostCS/HelpersHostCS.psd1 Outdated

@TravisEz13 TravisEz13 Apr 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can be removed

@iSazonov iSazonov Apr 23, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

TravisEz13
TravisEz13 reviewed Apr 23, 2017
View reviewed changes
Comment thread test/tools/Modules/HelpersLanguage/HelpersLanguage.psd1 Outdated

@TravisEz13 TravisEz13 Apr 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

see previous comment

@iSazonov iSazonov Apr 23, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

TravisEz13
TravisEz13 reviewed Apr 23, 2017
View reviewed changes
Comment thread test/tools/Modules/HelpersRemoting/HelpersRemoting.psd1 Outdated

@TravisEz13 TravisEz13 Apr 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

see previous comment

@iSazonov iSazonov Apr 23, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

TravisEz13
TravisEz13 reviewed Apr 23, 2017
View reviewed changes

@TravisEz13 TravisEz13 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't see any major issues.

@iSazonov

iSazonov commented Apr 28, 2017

Copy link
Copy Markdown
Collaborator Author

@TravisEz13 Could you please continue with the PR?

@TravisEz13

TravisEz13 commented May 10, 2017

Copy link
Copy Markdown
Member

@iSazonov Sorry, I've been out of the office for a few weeks.

@TravisEz13 TravisEz13 closed this May 10, 2017
@TravisEz13 TravisEz13 reopened this May 10, 2017
@TravisEz13

TravisEz13 commented May 10, 2017

Copy link
Copy Markdown
Member

Closed and re-opened PR to re-trigger CI. Could you rebase as well?

@iSazonov iSazonov force-pushed the tests-auto-ipmo-3 branch from eccc5c3 to 4c7b812 Compare May 11, 2017 05:10
@iSazonov

iSazonov commented May 11, 2017

Copy link
Copy Markdown
Collaborator Author

Rebase done.

@iSazonov

iSazonov commented May 16, 2017

Copy link
Copy Markdown
Collaborator Author

@TravisEz13 Is the PR ready to merge?

@TravisEz13 TravisEz13 merged commit e00161a into PowerShell:master May 17, 2017
@TravisEz13

TravisEz13 commented May 17, 2017

Copy link
Copy Markdown
Member

Yeah @iSazonov, Thanks. I was giving people time to give feedback. Thanks for pinging me too.

@adityapatwardhan adityapatwardhan mentioned this pull request May 18, 2017
Merged
@iSazonov iSazonov deleted the tests-auto-ipmo-3 branch September 28, 2018 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

+1 more reviewer

@lzybkr lzybkr lzybkr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@TravisEz13 TravisEz13

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@iSazonov @lzybkr @TravisEz13 @msftclas
Morty Proxy This is a proxified and sanitized view of the page, visit original site.