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.

Sign up for GitHub

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
Merged

[chore] Update windows tests component groups #32518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5e1193f
Update windows tests component groups
TylerHelmuth Apr 18, 2024
fd7df99
Skip tests
TylerHelmuth Apr 18, 2024
27bf78c
Update windowseventlog test config
TylerHelmuth Apr 18, 2024
e933a09
Fix windowsperfcounter test
TylerHelmuth Apr 18, 2024
4ab8ef1
Update receiver/windowsperfcountersreceiver/factory_windows.go
TylerHelmuth Apr 18, 2024
f899222
Update receiver/windowseventlogreceiver/metadata.yaml
TylerHelmuth Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions .github/workflows/build-and-test-windows.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,22 @@ concurrency:
jobs:
windows-unittest-matrix:
strategy:
fail-fast: false
matrix:
group:
- receiver-0
- receiver-1
- receiver-2
- receiver-3
- processor
- exporter
- exporter-0
- exporter-1
- extension
- connector
- internal
- pkg
- cmd
- cmd-0
- cmd-1
- other
runs-on: windows-latest
if: ${{ github.actor != 'dependabot[bot]' && (contains(github.event.pull_request.labels.*.name, 'Run Windows') || github.event_name == 'push' || github.event_name == 'merge_group') }}
Expand All @@ -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.

name: install IIS
- name: install IIS
run: Install-WindowsFeature -name Web-Server -IncludeManagementTools
- uses: actions/setup-go@v5
with:
Expand All @@ -61,9 +66,6 @@ jobs:
~\go\pkg\mod
~\AppData\Local\go-build
key: go-build-cache-${{ runner.os }}-${{ matrix.group }}-go-${{ hashFiles('**/go.sum') }}
- if: matrix.group == 'cmd'
name: Increasing GOTEST_TIMEOUT for group 'cmd'
run: echo "GOTEST_TIMEOUT=1200s" >> $Env:GITHUB_ENV
- name: Run Unit tests
run: make -j2 gotest GROUP=${{ matrix.group }}
windows-unittest:
Expand Down
9 changes: 9 additions & 0 deletions receiver/sqlserverreceiver/queries_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,17 @@ package sqlserverreceiver
import (
"os"
"path"
"runtime"
"testing"

"github.com/stretchr/testify/require"
)

func TestQueryIODBWithoutInstanceName(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("Test is failing on Windows, see https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/32519")
}

expected, err := os.ReadFile(path.Join("./testdata", "databaseIOQueryWithoutInstanceName.txt"))
require.NoError(t, err)

Expand All @@ -21,6 +26,10 @@ func TestQueryIODBWithoutInstanceName(t *testing.T) {
}

func TestQueryIODBWithInstanceName(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("Test is failing on Windows, see https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/32519")
}

expected, err := os.ReadFile(path.Join("./testdata", "databaseIOQueryWithInstanceName.txt"))
require.NoError(t, err)

Expand Down
2 changes: 2 additions & 0 deletions receiver/windowseventlogreceiver/metadata.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,5 @@ status:

tests:
config:
start_at: end
channel: application
4 changes: 3 additions & 1 deletion receiver/windowsperfcountersreceiver/factory_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import (
"go.opentelemetry.io/collector/consumer"
"go.opentelemetry.io/collector/receiver"
"go.opentelemetry.io/collector/receiver/scraperhelper"

"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/windowsperfcountersreceiver/internal/metadata"
)

// createMetricsReceiver creates a metrics receiver based on provided config.
Expand All @@ -25,7 +27,7 @@ func createMetricsReceiver(
scraper := newScraper(oCfg, params.TelemetrySettings)

scrp, err := scraperhelper.NewScraper(
params.ID.String(),
metadata.Type.String(),
scraper.scrape,
scraperhelper.WithStart(scraper.start),
scraperhelper.WithShutdown(scraper.shutdown),
Expand Down
Loading