Skip to content

[hotfix] Fix testfail due to data race with after migration to Fabric8 interceptor #891

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[hotfix] Fix testfail due to data race with after migration to Fabric8 interceptor #891

wants to merge 1 commit into from

Conversation

@aplyusnin
Copy link

@aplyusnin aplyusnin commented Oct 9, 2024

Tests that perform requests are usualy fail now on CI due to race conditions.

To fix it, I addeв busywait for metrics with timeout as it is done in RestApiMetricsCollectorTest#testJmMetricCollection().
Also I added an extra delay in the RestApiMetricsCollectorTest#testJmMetricCollection(), it helps to avoid CI failing on my workload runs.

Alternative fix:
Add blocks inside tests until requests are performed, but this requreis a lot of modifications

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changes to the CustomResourceDescriptors: no
  • Core observer or reconciler logic that is regularly executed: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not documented

SamBarker
SamBarker reviewed Oct 9, 2024
View reviewed changes
Copy link
Contributor

@SamBarker SamBarker left a comment

Choose a reason for hiding this comment

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

I usually treat thread.sleep as a smell in tests. Do you have a theory as to why its needed?

I'll try and investigate but its nots a problem I observed when running the tests while doing the interceptor work.

@aplyusnin
Copy link
Author

aplyusnin commented Oct 10, 2024 •
edited
Loading

kubernetesClient.resource(deployment).delete() has to be a blocking call, but when I run the tests with some extra logging I saw, that 404 response code doesn't appear before assertions on CI (I don't actually know why it is, I didn't get that problem locally). After some experiments, I came up with a conclusion, that such way fixes the problem.

I agree, that it is not good way for fixing the problem, but I don't know how to properly add some extra blocking for the main testing Thread to get rid of the problem.

@SamBarker SamBarker mentioned this pull request Oct 11, 2024
Merged
@SamBarker
Copy link
Contributor

SamBarker commented Oct 11, 2024

I've raised #896 to actually wait for the metrics to become available.

@gyfora
Copy link
Contributor

gyfora commented Oct 11, 2024

I think with @SamBarker 's PR merged, we can close this one right?

@aplyusnin
Copy link
Author

aplyusnin commented Oct 11, 2024

Yes, I close it

@aplyusnin aplyusnin closed this Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@SamBarker SamBarker SamBarker left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aplyusnin @SamBarker @gyfora