Skip to content
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

Merged
merged 4 commits into from Apr 19, 2024
Merged

Conversation

fflaten
Copy link
Collaborator

@fflaten fflaten commented Jun 7, 2023

PR Summary

All blocks and tests have an Id-property used to group tests/blocks generated using the It/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 of It/Context/Describe to a generated guid. Block.Id and Test.Id as marked as obsolete for now and is used as aliases for the new GroupId-value.

Fix #2333

PR Checklist

  • PR has meaningful title
  • Summary describes changes
  • 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.
  • Tests are added/update (if required)
  • Documentation is updated/added (if required)

@fflaten
Copy link
Collaborator Author

fflaten commented Jun 7, 2023

Tests failed because Group-Object sorts by name, so with guids the order is unpredictable in nunit-reports when we group the tests/blocks. Need to either filter by name in tests or reconsider value.

Combine old and new "$($_.StartLine)-$guid"?

@fflaten
Copy link
Collaborator Author

fflaten commented Jan 29, 2024

Changing Id/GroupId from StartLine to random value like a guid would become a minor breaking change.

Given Describe 'd' -ForEach @(1,2) { It 'i' -ForEach @(1,2) { } }, $result.Tests | ? Id | Group-Object { "$($_.Block.BlockContainer.Item):$($_.Id)" } would return:

  • Before PR: A single group with count 4
  • After PR: Two groups with count 2, one per block.

The after behavior is the intended behavior AFAIK. The workaround for any affected users would be to use $result.Tests | ? Id | Group-Object { "$($_.Block.BlockContainer.Item):$($_.StartLine)" }.

@nohwnd
Copy link
Member

nohwnd commented Apr 12, 2024

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.

@fflaten
Copy link
Collaborator Author

fflaten commented Apr 12, 2024

Guids are long and complicated, super hard to remember and dynamic.

Id/GroupId doesn't have to be static/reproducible or something you'd reference, does it? Only used to group data-generated blocks/tests, or am I missing something? :)

Also we use line number in line filter, I don't remember if this would be affected.

It uses the StartLine-property

Is there a problem with line number as id?

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 StartLine.

A value of startline:startposition should suffice though, as mentioned in the original comment and solves the test sorting issue. Will update soon. 🙂

@nohwnd
Copy link
Member

nohwnd commented Apr 16, 2024

Id/GroupId doesn't have to be static/reproducible or something you'd reference, does it?

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.

@nohwnd nohwnd marked this pull request as ready for review April 18, 2024 07:50
@fflaten
Copy link
Collaborator Author

fflaten commented Apr 18, 2024

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 ...Tests | ? GroupId | Group { $_.Block.GetHashCode() }, but should a group id not indicate the exact group?

@nohwnd
Copy link
Member

nohwnd commented Apr 19, 2024

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:

4:72
   4:104
   4:104
   4:133
   4:133
4:72
   4:104
   4:104
   4:133
   4:133
4:163
   4:195
   4:195
4:163
   4:195
   4:195

And when formatting the code on multiple lines:

5:1
   6:5
   6:5
   7:5
   7:5
5:1
   6:5
   6:5
   7:5
   7:5
9:1
   10:5
   10:5
9:1
   10:5
   10:5

We had 2 Describe blocks and have 2 unique ids on the first level. And we had 3 It blocks and have 3 different ids on the second level. So all is fine.

Before the change we would have all just be 4:

4
  4
  4
  4
  4
4
  etc..

So the first two It blocks would be wrongly merged into the same group in report.

@nohwnd nohwnd merged commit 9d44623 into pester:main Apr 19, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename Id-property for parameterized items
2 participants