-
-
Notifications
You must be signed in to change notification settings - Fork 476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace Id with GroupId in Block and Test objects #2364
Conversation
Tests failed because Combine old and new |
Changing Id/GroupId from StartLine to random value like a guid would become a minor breaking change. Given
The after behavior is the intended behavior AFAIK. The workaround for any affected users would be to use |
Is there a problem with line number as id? Guids are long and complicated, super hard to remember and dynamic. Line ids are mostly static, and short. Also we use line number in line filter, I don't remember if this would be affected. |
It uses the
Conflict with multiple groups on same like, typically debug/testing oneliner containers. Felt resonable to fix that while modifying this part. Can't remember why I when with a guid, maybe to avoid making it look redundant due to having the same value as A value of |
It does not have to be, but if it is it's much easier to debug, especially if you need more attempts. So I would stick with line number, or line:column number if we can. I can fit 3 of these in my head, I cannot fit 3 guids that change in my head. |
Was refreshing this PR yesterday and got reminded why I chose the random id. With only line:offset you're unable to identity tests coming from the same context (parent block/data), see #2364 (comment) A workaround would be |
The id/groupId is just to put together tests that were generated from the same call to Describe / It, so reports can know which tests are coming from the same logical "test group" but we don't have to wrap them into additional layer in the result object. The id needs to be unique only within the current block. It does not have to be globally unique. $result = invoke-pester -Container (New-PesterContainer -ScriptBlock { Describe 'd' -ForEach @(1,2) { It 'i' -ForEach @(1,2) { }; It 'j' -ForEach @(1,2) { } }; Describe 'd1' -ForEach @(3,4) { It 'i' -ForEach @(3,4) { } } }) -passthru -output detailed
$result.Containers.Blocks | % { $_.GroupId ; $_.Tests | %{ " $($_.GroupId)"} } This single line test run shows:
And when formatting the code on multiple lines:
We had 2 Before the change we would have all just be 4:
So the first two It blocks would be wrongly merged into the same group in report. |
PR Summary
All blocks and tests have an
Id
-property used to group tests/blocks generated using theIt/Context/Describe
using-ForEach
.This PR replaces with a new
GroupId
property to make it more explicit. It also changes the value from start line ofIt/Context/Describe
to a generated guid.Block.Id
andTest.Id
as marked as obsolete for now and is used as aliases for the newGroupId
-value.Fix #2333
PR Checklist
Create Pull Request
to mark it as a draft. PR can be markedReady for review
when it's ready.