Skip to content

Add autoload for TestLanguage.psm1 TestHelpers.psm1#3456

Merged
TravisEz13 merged 7 commits intoPowerShell:masterfrom
iSazonov:tests-auto-ipmo-3
May 17, 2017
Merged

Add autoload for TestLanguage.psm1 TestHelpers.psm1#3456
TravisEz13 merged 7 commits intoPowerShell:masterfrom
iSazonov:tests-auto-ipmo-3

Conversation

@iSazonov
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov commented Mar 30, 2017

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
Copy link
Copy Markdown
Contributor

@lzybkr lzybkr left a comment

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
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Apr 4, 2017 •
edited
Loading

Can we add a "Module" suffix?

  • TestCommonHelpersModule
  • TestLanguageModule
  • TestHostCSModule
  • TestRemotingModule

@lzybkr
Copy link
Copy Markdown
Contributor

lzybkr commented Apr 4, 2017 •
edited
Loading

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
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Apr 4, 2017

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
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Apr 6, 2017

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

@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Apr 13, 2017

@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
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Apr 19, 2017

@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
test/tools/Modules/HelpersCommon/HelpersCommon.psd1 Outdated
Copy link
Copy Markdown
Member

@TravisEz13 TravisEz13 Apr 23, 2017

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
test/tools/Modules/HelpersHostCS/HelpersHostCS.psd1 Outdated
Copy link
Copy Markdown
Member

@TravisEz13 TravisEz13 Apr 23, 2017

Choose a reason for hiding this comment

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

can be removed

Copy link
Copy Markdown
Collaborator Author

@iSazonov iSazonov Apr 23, 2017

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
test/tools/Modules/HelpersLanguage/HelpersLanguage.psd1 Outdated
Copy link
Copy Markdown
Member

@TravisEz13 TravisEz13 Apr 23, 2017

Choose a reason for hiding this comment

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

see previous comment

Copy link
Copy Markdown
Collaborator Author

@iSazonov iSazonov Apr 23, 2017

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
test/tools/Modules/HelpersRemoting/HelpersRemoting.psd1 Outdated
Copy link
Copy Markdown
Member

@TravisEz13 TravisEz13 Apr 23, 2017

Choose a reason for hiding this comment

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

see previous comment

Copy link
Copy Markdown
Collaborator Author

@iSazonov iSazonov Apr 23, 2017

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
Copy link
Copy Markdown
Member

@TravisEz13 TravisEz13 left a comment

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
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Apr 28, 2017

@TravisEz13 Could you please continue with the PR?

@TravisEz13
Copy link
Copy Markdown
Member

TravisEz13 commented May 10, 2017

@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
Copy link
Copy Markdown
Member

TravisEz13 commented May 10, 2017

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
Copy link
Copy Markdown
Collaborator Author

iSazonov commented May 11, 2017

Rebase done.

@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented May 16, 2017

@TravisEz13 Is the PR ready to merge?

@TravisEz13 TravisEz13 merged commit e00161a into PowerShell:master May 17, 2017
@TravisEz13
Copy link
Copy Markdown
Member

TravisEz13 commented May 17, 2017

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