FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add autoload for TestLanguage.psm1 TestHelpers.psm1 by iSazonov · Pull Request #3456 · PowerShell/PowerShell · GitHub

/ PowerShell Public

Add autoload for TestLanguage.psm1 TestHelpers.psm1 - #3456

Merged
Travis Plunk (TravisEz13) merged 7 commits into
PowerShell:masterfrom
iSazonov:tests-auto-ipmo-3
May 17, 2017
ConversationCommits (7)ChecksFiles changed
Merged

Add autoload for TestLanguage.psm1 TestHelpers.psm1#3456
Travis Plunk (TravisEz13) merged 7 commits into
PowerShell:masterfrom
iSazonov:tests-auto-ipmo-3

Conversation

Ilya (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)

Jason Shirk (lzybkr) reviewed Apr 4, 2017
View reviewed changes

Jason Shirk (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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Ilya (iSazonov) commented Apr 4, 2017 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Can we add a "Module" suffix?

  • TestCommonHelpersModule
  • TestLanguageModule
  • TestHostCSModule
  • TestRemotingModule

Jason Shirk (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.

Ilya (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

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

Ilya (iSazonov) commented Apr 6, 2017

Copy link
Copy Markdown
Collaborator Author

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

Ilya (iSazonov) commented Apr 13, 2017

Copy link
Copy Markdown
Collaborator Author

Jason Shirk (@lzybkr) Travis Plunk (@TravisEz13) Do I need something else to do here?

Ilya (iSazonov) commented Apr 19, 2017

Copy link
Copy Markdown
Collaborator Author

Jason Shirk (@lzybkr) Travis Plunk (@TravisEz13) Could you please review and merge? This blocks further work.

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

Travis Plunk (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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There are still comments here

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

Travis Plunk (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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

can be removed

Ilya (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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed.

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

Travis Plunk (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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

see previous comment

Ilya (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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed.

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

Travis Plunk (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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

see previous comment

Ilya (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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed.

Travis Plunk (TravisEz13) reviewed Apr 23, 2017
View reviewed changes

Travis Plunk (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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't see any major issues.

Ilya (iSazonov) commented Apr 28, 2017

Copy link
Copy Markdown
Collaborator Author

Travis Plunk (@TravisEz13) Could you please continue with the PR?

Travis Plunk (TravisEz13) commented May 10, 2017

Copy link
Copy Markdown
Member

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

Travis Plunk (TravisEz13) commented May 10, 2017

Copy link
Copy Markdown
Member

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

Ilya (iSazonov) commented May 11, 2017

Copy link
Copy Markdown
Collaborator Author

Rebase done.

Ilya (iSazonov) commented May 16, 2017

Copy link
Copy Markdown
Collaborator Author

Travis Plunk (@TravisEz13) Is the PR ready to merge?

Travis Plunk (TravisEz13) merged commit e00161a into PowerShell:master May 17, 2017

Travis Plunk (TravisEz13) commented May 17, 2017

Copy link
Copy Markdown
Member

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

Merged
Ilya (iSazonov) deleted the tests-auto-ipmo-3 branch September 28, 2018 12:33
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
Add autoload for TestLanguage.psm1 TestHelpers.psm1 (PowerShell#3456)
0ee7557 
* Add autoload for TestHelpers.psm1

Test.Helpers.psm1 was renamed to TestHelpers.psm1

* Resolve conflit and rebase Add autoload for TestLanguage.psm1

* Remove unneeded comments from PSD1 files

* Rename test modules

Remove approved verbs (Get-Verb) from module names.

* Enhance ShouldBeErrorId to output exception into pipeline for later analysis

* Remove unneeded comments

* Resolve merge conflict
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Travis Plunk (TravisEz13) TravisEz13 approved these changes
James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
+1 more reviewer
Jason Shirk (lzybkr) lzybkr left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

Travis Plunk (TravisEz13)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL