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

[pkg/stanza] Fileconsumer test TestExcludeOlderThanFilter/exclude_some_files failing #32838

Open
crobert-1 opened this issue May 2, 2024 · 13 comments
Labels
never stale Issues marked with this label will be never staled and automatically removed os:windows pkg/stanza

Comments

@crobert-1
Copy link
Member

crobert-1 commented May 2, 2024

Component(s)

pkg/stanza

Describe the issue you're reporting

CI/CD failure link

Note that this failure was on Windows

=== FAIL: fileconsumer/matcher/internal/filter TestExcludeOlderThanFilter/exclude_some_files (0.01s)
    exclude_test.go:114: 
        	Error Trace:	D:/a/opentelemetry-collector-contrib/opentelemetry-collector-contrib/pkg/stanza/fileconsumer/matcher/internal/filter/exclude_test.go:114
        	Error:      	Not equal: 
        	            	expected: []string{"a.log"}
        	            	actual  : []string{"a.log", "b.log"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,3 +1,4 @@
        	            	-([]string) (len=1) {
        	            	- (string) (len=5) "a.log"
        	            	+([]string) (len=2) {
        	            	+ (string) (len=5) "a.log",
        	            	+ (string) (len=5) "b.log"
        	            	 }
        	Test:       	TestExcludeOlderThanFilter/exclude_some_files
@crobert-1 crobert-1 added needs triage New item requiring triage pkg/stanza labels May 2, 2024
Copy link
Contributor

github-actions bot commented May 2, 2024

Pinging code owners for pkg/stanza: @djaglowski. See Adding Labels via Comments if you do not have permissions to add labels yourself.

Copy link
Contributor

github-actions bot commented May 2, 2024

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@djaglowski
Copy link
Member

cc @ycombinator

@ycombinator
Copy link
Contributor

Thanks @djaglowski, will look into this ASAP.

@ycombinator
Copy link
Contributor

ycombinator commented May 3, 2024

This is not failing for me locally (on a Mac) so this is likely a Windows-specific issue.

# On my Mac
$ go test ./fileconsumer/matcher/internal/filter/... -test.run TestExcludeOlderThanFilter -count 1000
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/fileconsumer/matcher/internal/filter	3.130s

Also, I'm not seeing this test fail regularly in CI so it looks to be a flaky test.

@djaglowski
Copy link
Member

Thanks @ycombinator. Makes sense that it's a flaky test otherwise it probably would have prevented us merging the PR.

I think if it's not a quick fix we should skip the test for now. Either way, we'll need to get to the bottom of it and fix it.

@ycombinator
Copy link
Contributor

I think if it's not a quick fix we should skip the test for now. Either way, we'll need to get to the bottom of it and fix it.

The fix might be quick but I don't know when I'll have time to test this out on Windows. So in that sense, it might not be a quick fix.

Instead of skipping that test case entirely, perhaps we could only skip it on Windows, referencing this issue here? I can put up a PR for that pretty quickly.

@djaglowski
Copy link
Member

Good idea, thank you!

@ycombinator
Copy link
Contributor

ycombinator commented May 3, 2024

PR to skip the flaky test only on Windows: #32848

djaglowski pushed a commit that referenced this issue May 3, 2024
…lude_some_files` on Windows (#32848)

**Description:** <Describe what has changed.>
This PR skips the test case
`TestExcludeOlderThanFilter/exclude_some_files` only on Windows because
it appears to be flaky there.

**Link to tracking Issue:** <Issue number if applicable>

#32838
@ChrsMark
Copy link
Member

It seems this issue is legit. Shall we remove the needs triage label?

@djaglowski djaglowski removed the needs triage New item requiring triage label May 16, 2024
@ycombinator ycombinator removed their assignment May 16, 2024
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Jul 16, 2024
@ChrsMark
Copy link
Member

Still valid.

@djaglowski djaglowski added never stale Issues marked with this label will be never staled and automatically removed and removed Stale labels Jul 16, 2024
@djaglowski
Copy link
Member

I think this ultimately requires a solution to the windows timing issue. More details on this issue.

djaglowski pushed a commit that referenced this issue Jul 23, 2024
…anFilter (#34174)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Similar to
#34128.
Mocking the time functions to fix flakiness on windows.

**Link to tracking Issue:** <Issue number if applicable>
#32838

**Testing:** <Describe what testing was performed and which tests were
added.>

**Documentation:** <Describe the documentation added.>

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
never stale Issues marked with this label will be never staled and automatically removed os:windows pkg/stanza
Projects
None yet
Development

No branches or pull requests

4 participants