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

[chore] Update windows tests component groups #32518

Merged
merged 6 commits into from
Apr 18, 2024

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Apr 18, 2024

Description:
Someone, and we may never know who, forgot to update the windows test groups when they created #30901

Fixes #32508

@TylerHelmuth TylerHelmuth requested review from a team and fatsheep9146 April 18, 2024 14:55
@TylerHelmuth TylerHelmuth added the Run Windows Enable running windows test on a PR label Apr 18, 2024
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

I'm glad there's no way at all to dig into the history of the code to figure out who to blame 😆

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Another PR is in flight that changes the exporter groups. Depending on which one is merged first, the other will need modified. I'm adding a comment there to update accordingly.

#32496

@songy23 songy23 added the ci-cd CI, CD, testing, build issues label Apr 18, 2024
@crobert-1
Copy link
Member

I've submitted #32520 to resolve the failing SQL Server unit tests on Windows.

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

It seems that more tests need to be skipped.

@@ -45,8 +51,7 @@ jobs:
GOMEMLIMIT: 2GiB
steps:
- uses: actions/checkout@v4
- if: matrix.group == 'receiver-0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the conditional? Is it needed for something other than receiver-0 group?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe it is needed for the iis receiver, which changed to group 1, showing how the condition is fragile. I removed it so that in the future if the groups change again we wont have to worry about updating this condition. That does mean that all the groups get IIS installed. Since the tests don't run that often I thought that trade off was worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It adds something like 50s to 1m30s to each Windows unit test run I tend to see as waste, but, overall I think you are right: this extra time and compute seems acceptable in exchange for the consistency.

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

LGTM as soon as any pending test skips are added

@TylerHelmuth TylerHelmuth merged commit 169e1c2 into open-telemetry:main Apr 18, 2024
169 checks passed
@TylerHelmuth TylerHelmuth deleted the fix-windows-tests branch April 18, 2024 17:40
@github-actions github-actions bot added this to the next release milestone Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[receiver/windowsperfcounters] Unit tests are not running on CI
7 participants