Allow using 'RootModule/NestedModule' as ModuleName

pester / Pester

Pester is the ubiquitous test and mock framework for PowerShell.

https://pester.dev/

Other

3.08k stars 470 forks source link

Allow using 'RootModule/NestedModule' as ModuleName #2412

Closed fred-gagnon closed 2 months ago

fred-gagnon commented 9 months ago

PR Summary

Updated function Get-CompatibleModule to allow specifying a module name as RootModule/NestedModule. This change solves an issue where multiple modules with a similarly named nested module cause an exception Multiple script or manifest modules named... to be thrown when attempting to create a mock in a nested module.

PR Checklist

[X] PR has meaningful title
[X] Summary describes changes
[X] PR is ready to be merged
- If not, use the arrow next to Create Pull Request to mark it as a draft. PR can be marked Ready for review when it's ready.
[X] Tests are added/update (if required)
[ ] Documentation is updated/added (if required)

fflaten commented 8 months ago

Thanks for the PR. Will review this soon. Related https://github.com/pester/Pester/issues/2235#issuecomment-1264617847

fred-gagnon commented 6 months ago

Thanks for the PR. Will review this soon. Related #2235 (comment)

Any idea when it will be reviewed ?

fflaten commented 6 months ago

Sorry for the delay. I've been fully occupied at work for a while but will hopefully get some time for OSS this month.

I'm also waiting on @nohwnd to return (find time). He'll need to eventually merge and release the pending PRs once approved.

nohwnd commented 5 months ago

Can you please add a test that uses InModuleScope with this syntax and proves that we ended up in the correct context? E.g. by checking $ExecutionContext.SessionState.Module.Name or a value of a variable.

This would also serve as an example usage.

fred-gagnon commented 5 months ago

@nohwnd The tests you requested have been added

fflaten commented 5 months ago

@nohwnd Won't this also allow mocking, while cleanup will fail?

Haven't had the time to test it yet, but that's a concern I had.

nohwnd commented 5 months ago

Looks like it would impact Mock, because the function is common. @fred-gagnon can you please allow this only from InModuleScope (e.g. by bool parameter that needs to be provided)? I am not sure what the impact is on mocking, and I won't be able to deeply investigate this any time soon.

fred-gagnon commented 5 months ago

@nohwnd The whole point of this PR is for use-cases I have where I need to inject a Mock in a sub-module and there is more than one sub-modules currently loaded which have the same name, for example when 2 REST clients are simultaneously loaded and each one has a Repository sub-module. If I am to limit the use of this change only for InModuleScope, then it is pointless to keep it open. Unless I can use Mock from within InModuleScope? I’ve never tried that and won’t be able to for a few days.

nohwnd commented 5 months ago

I don't know why I was under the impression that this is for InModuleScope, sorry.

To allow this change for Mocking you need to add ton of tests mocks. I am not trying to be difficult, but we rely on simple name to figure out location of mocks. We also don't inspect child modules to clean them etc. So basically no-one knows what the change would cause (see comment: https://github.com/pester/Pester/issues/2235#issuecomment-1264617847 ), and your scenario is so rare that it does not seem worth it to break the usage for everyone else.

If you want to take it all the way through then I am okay with it, but it won't be easy. 🙂

nohwnd commented 2 months ago

Closing due to no activity on this PR, taking it to completion would require the implementation described above, if you want to commit to that please let me know.