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

Jump to bottom

Rerun a flaky test #639

Merged
merged 6 commits into from
Aug 30, 2021
Merged

Rerun a flaky test #639

merged 6 commits into from
Aug 30, 2021

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Aug 30, 2021 •
edited
Loading

Workaround for #638

@ocelotl ocelotl self-assigned this Aug 30, 2021
@ocelotl ocelotl requested review from a team, owais and lzchen and removed request for a team August 30, 2021 15:27
@ocelotl ocelotl added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 30, 2021
@owais
Copy link
Contributor

owais commented Aug 30, 2021

This can help but I don't think it should be considered a fix for #638. We should actually fix the flaky test to close that ticket.

codeboten
codeboten approved these changes Aug 30, 2021
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

I think its fine to skip the flaky test, but the PR should not be marked as a fix for the problem.

tests/opentelemetry-docker-tests/tests/celery/test_celery_functional.py
@@ -558,6 +558,8 @@ class CelerySubClass(CelerySuperClass):
) == async_run_span.attributes.get(SpanAttributes.MESSAGING_MESSAGE_ID)


# FIXME find a permanent solution for the flakiness of this test
Copy link
Contributor

Choose a reason for hiding this comment

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

If the fix for the issue includes a FIXME, then I agree w/ @owais that this doesn't fix #638 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the description of this PR 👍

@owais owais merged commit c279ee5 into open-telemetry:main Aug 30, 2021
@ocelotl ocelotl deleted the issue_638 branch August 31, 2021 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzchen lzchen lzchen approved these changes

@codeboten codeboten codeboten approved these changes

@owais owais owais approved these changes

Assignees

@ocelotl ocelotl

Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ocelotl @owais @codeboten @lzchen