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(logging): handle TestListLogEntriesRequest flakiness #5460

Merged
merged 6 commits into from Feb 23, 2022

Conversation

minherz
Copy link
Contributor

@minherz minherz commented Feb 7, 2022

remove timestamp comparison which fails due to (milli-)seconds mismatch when executing time.Now() to get filter time delayed after time.Now() in the test.
separately test default (-24h) look back period.

Fixes #5441

@minherz minherz requested review from a team as code owners February 7, 2022 08:16
@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: logging Issues related to the Cloud Logging API. labels Feb 7, 2022
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

Just a couple of things

logging/logadmin/logadmin_test.go Outdated Show resolved Hide resolved
logging/logadmin/logadmin_test.go Outdated Show resolved Hide resolved
logging/logadmin/logadmin_test.go Show resolved Hide resolved
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

Just a couple more error reports to flip

}
filterTime, err := time.Parse(time.RFC3339, strings.Trim(got.Filter[len(timeFilterPrefix):], "\""))
if err != nil {
t.Errorf("%v: expect default filter to have time in RFC3339; got %v", err, got.Filter)
Copy link
Member

Choose a reason for hiding this comment

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

got, want


// Default is client's project ID, 24 hour lookback, and no orderBy.
if !testutil.Equal(got.ResourceNames, []string{"projects/PROJECT_ID"}) || got.OrderBy != "" || timeDiff.Hours() < 24 {
t.Errorf("expect {\"projects/PROJECT_ID\"} resource names, 24 hour look back filter and native order\ngot %v", got)
Copy link
Member

Choose a reason for hiding this comment

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

got, want

remove timestamp comparison which fails due to (milli-)seconds mismatch when
executing time.Now() to get filter time delayed after time.Now() in the test.
separately test default (-24h) look back period
align error message with expected format
refactor testing params for TestListLogEntriesRequest
@minherz minherz requested a review from codyoss February 14, 2022 09:39
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

Thanks!

@minherz minherz merged commit 6b5db1b into main Feb 23, 2022
@minherz minherz deleted the minherz/fix-5441 branch February 23, 2022 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the Cloud Logging API. size: xs Pull request size is extra small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

logging/logadmin: TestListLogEntriesRequest failed
2 participants