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] Force require.EventuallyWithT to fail properly #35032

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

djaglowski
Copy link
Member

I noticed on #34720 and #35026 that execution of the test continued beyond a failure of require.EventuallyWithT. Based on the description alone, I would expect that using assert within require.EventuallyWithT should cause execution to stop if the assertion fails, but it appears this may not be the case. However, this change apparently works as intended.

@djaglowski djaglowski added the Run Windows Enable running windows test on a PR label Sep 5, 2024
@djaglowski
Copy link
Member Author

I'm not sure this should be merged until we have a more wholistic understanding of the failures mentioned here.

@pjanotti
Copy link
Contributor

I agree @djaglowski - Unfortunately, after that initial series of failures that I mentioned in the comment that you linked, the failures went away without any clear action on my part. Reviewing the code I detected a small thing that should be changed as precaution: if the first run of the tests fails without running the deferred code, the second run will fail on the registry entry already being present so no second run. I'm not convinced that it is a real problem in practice, but, let me clear that in quick PR and afterwards I will monitor the failures to see if we can get a better grip on what is going on with these tests.

@pjanotti
Copy link
Contributor

Created #35168 to handle the comment above.

@djaglowski
Copy link
Member Author

This change still appears to surface a failure:

=== FAIL: . TestReadWindowsEventLogger (unknown)
panic: Assertion failed

goroutine 21 [running]:
github.com/stretchr/testify/assert.(*CollectT).FailNow(0x1417376c0?)
	C:/Users/runneradmin/go/pkg/mod/github.com/stretchr/testify@v1.9.0/assert/assertions.go:1924 +0x2b
github.com/stretchr/testify/require.Len({0x14173b120, 0xc0001121f8}, {0x14136fd80, 0x141f8d6a0}, 0x1, {0x0, 0x0, 0x0})
	C:/Users/runneradmin/go/pkg/mod/github.com/stretchr/testify@v1.9.0/require/require.go:1159 +0xf1
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/windowseventlogreceiver.requireExpectedLogRecords.func1(0xc0001121f8)
	D:/a/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/windowseventlogreceiver/receiver_windows_test.go:330 +0x131
github.com/stretchr/testify/assert.EventuallyWithT.func1()
	C:/Users/runneradmin/go/pkg/mod/github.com/stretchr/testify@v1.9.0/assert/assertions.go:1983 +0xb8
created by github.com/stretchr/testify/assert.EventuallyWithT in goroutine 15
	C:/Users/runneradmin/go/pkg/mod/github.com/stretchr/testify@v1.9.0/assert/assertions.go:1978 +0x42b

I think this is legitimate because #35026 fails as well, but in a less clear way because the actual failure is ignored.

@pjanotti
Copy link
Contributor

@djaglowski let's merge this one and observe on the CI runs. One thing that I also want to do is to remove the single t.Parallel() because it makes the test using it to show in the failure that you linked, but, I think that it is just a red-herring. I will create a PR for that shortly.

@pjanotti
Copy link
Contributor

To remove t.Parallel() #35178

@djaglowski djaglowski merged commit 466b86e into open-telemetry:main Sep 13, 2024
170 of 172 checks passed
@djaglowski djaglowski deleted the wel-require-fail branch September 13, 2024 17:13
@github-actions github-actions bot added this to the next release milestone Sep 13, 2024
pjanotti added a commit to pjanotti/opentelemetry-service-contrib that referenced this pull request Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
receiver/windowseventlog Run Windows Enable running windows test on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants