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

Test Fedora Messaging messages in CI #3180

Merged
merged 2 commits into from Jun 20, 2019
Merged

Test Fedora Messaging messages in CI #3180

merged 2 commits into from Jun 20, 2019

Conversation

abompard
Copy link
Member

This PR adds the infrastructure to send and receive Fedora Messaging messages in the CI environment, and test the messages that went through.

@abompard abompard requested a review from a team as a code owner April 26, 2019 15:37
@abompard abompard added this to To do in CI Gating via automation Apr 26, 2019
@abompard abompard moved this from To do to Need Review in CI Gating Apr 26, 2019
@abompard
Copy link
Member Author

What can be added to this:

  • add a wrapper to send messages from the CI tests
  • add tests for the consumers

@abompard
Copy link
Member Author

It looks like Jenkins is not running the integration tests, I don't know why. I'm looking into it but if you have an idea I'm all ears.

@abompard
Copy link
Member Author

Ah, that's because of #3157

@abompard abompard force-pushed the ci-fm branch 7 times, most recently from 62115e4 to 609bd8c Compare April 30, 2019 13:09
@abompard
Copy link
Member Author

abompard commented May 1, 2019

OK, the failures we have now are related to the DNF API breakage.

bowlofeggs
bowlofeggs suggested changes May 2, 2019
View reviewed changes
devel/ci/integration/rabbitmq/rabbitmqadmin Outdated Show resolved Hide resolved
devel/ci/integration/tests/fixtures/rabbitmq.py Outdated Show resolved Hide resolved
devel/ci/integration/tests/fixtures/rabbitmq.py Outdated Show resolved Hide resolved
devel/ci/integration/tests/fixtures/rabbitmq.py Outdated Show resolved Hide resolved
devel/ci/integration/tests/test_messages.py Show resolved Hide resolved
devel/ci/integration/tests/test_messages.py
output = "".join(line.decode("utf-8") for line in output)
assert output.endswith("Sending composer.start message\n")
# Give some time for the message to go around
time.sleep(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could use asyncio.wait() here so that we will wait up to a timeout but not always the length of the sleep here? This way we can react as the messages arrive, but also won't hang forever if they don't arrive. I once found a sleep statement in Bodhi that was causing its unit tests to take a good bit longer and got a pretty big boost by dropping it in d26d42a ☺ What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure how to do that. What event will trigger the completion of the future in wait()? I don't think we can bubble up that event from the container to the host, where the tests are run. Can we?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I didn't really consider the complexity of detecting changes from within the container.

@abompard
Copy link
Member Author

abompard commented May 7, 2019

OK I've made some changes in the last commit (to be squashed before merge). I removed rabbitmqadmin because it's not actually necessary for what I was using it for.

I don't know how to address the time.sleep() issue, though. I don't think tests are run asynchronously in pytest. I could implement a listener as a fixture but that would bring the fedora-messaging stack into the test host, and then it would require tests to be run through Twisted. It may be doable but I'm not sure it's worth it.

@abompard
Copy link
Member Author

abompard commented Jun 4, 2019 •
edited

I rebased the PR, I think this is ready for re-review.
The CI error is caused by an upstream bug I think.

@abompard abompard requested a review from bowlofeggs June 4, 2019 13:06
@cverna
Copy link
Contributor

cverna commented Jun 5, 2019

@abompard One small change needed otherwise LGTM

@abompard
Test Fedora Messaging messages in CI
cff6c26 
Signed-off-by: Aurélien Bompard <aurelien@bompard.org>
@abompard
Copy link
Member Author

OK, I implemented your change @cverna , and I rebased and squashed the commits. This should be good for re-review. Thanks.

cverna
cverna approved these changes Jun 20, 2019
View reviewed changes
Copy link
Contributor

@cverna cverna left a comment

Choose a reason for hiding this comment

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

LGTM

bowlofeggs
bowlofeggs approved these changes Jun 20, 2019
View reviewed changes
Copy link
Contributor

@bowlofeggs bowlofeggs left a comment

Choose a reason for hiding this comment

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

Thanks!

@bowlofeggs bowlofeggs added the WIP Work in progress label Jun 20, 2019
@bowlofeggs bowlofeggs removed the WIP Work in progress label Jun 20, 2019
@mergify mergify bot merged commit 7e3048b into fedora-infra:develop Jun 20, 2019
CI Gating automation moved this from Need Review to Merged to develop Jun 20, 2019
@cverna cverna moved this from Merged to develop to Deployed to production in CI Gating Jul 25, 2019
@abompard abompard deleted the ci-fm branch August 14, 2019 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowlofeggs bowlofeggs bowlofeggs approved these changes

@cverna cverna cverna approved these changes

Assignees
No one assigned
Labels
None yet
Projects
CI Gating
  
Deployed to production
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@abompard @cverna @bowlofeggs