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

Add CI job to run system tests #761

Merged
merged 7 commits into from
Dec 17, 2024
Merged

Conversation

callumforrester
Copy link
Contributor

Fixes #630

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.92%. Comparing base (86d9470) to head (7071fed).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #761   +/-   ##
=======================================
  Coverage   92.92%   92.92%           
=======================================
  Files          37       37           
  Lines        2063     2063           
=======================================
  Hits         1917     1917           
  Misses        146      146           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@callumforrester callumforrester marked this pull request as ready for review December 17, 2024 11:17
Copy link
Contributor

@ZohebShaikh ZohebShaikh left a comment

Choose a reason for hiding this comment

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

Looks good !

@@ -101,6 +118,7 @@ def clean_existing_tasks(client: BlueapiClient):
yield


@pytest.mark.xfail(reason="https://github.com/DiamondLightSource/blueapi/issues/676")
Copy link
Contributor

Choose a reason for hiding this comment

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

REQUIRES_AUTH = bool(os.environ.get("REQUIRES_AUTH", 0))
REQUIRES_AUTH_MESSAGE = """
Authentication credentials are required to run this test.
The test has been skipped because authentication is currently disabled.
For more details, see: https://github.com/DiamondLightSource/blueapi/issues/676.
To enable and execute these tests, set `REQUIRES_AUTH=1` and provide valid credentials.
"""
requires_auth = pytest.mark.skipif(not REQUIRES_AUTH, reason=REQUIRES_AUTH_MESSAGE)

and then decorated with

@requires_auth

This will help us to run the tests on the local system even if we are skipping them on the CI

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 like the longer message, will implement.

I'm not a fan of having special steps to make the tests work, under this you would have to somehow know that you need to do REQUIRES_AUTH=False tox -e system-tests. A dev environment that works in a predictable way out-of-box is better IMO. I'm also not hugely in favour of having the local tests work differently to the CI, we were in that situation before we made bluesky-stomp and it caused lots of debugging problems. I would rather mark as xfail, clearly showing we know there's a problem, and fix it when we can. On a practical level it makes no difference, following the instructions for the system tests locally will result in a success message.

Comment on lines 66 to 75
attempts_remaining = 20
while attempts_remaining > 0:
try:
client.get_environment()
return
except ConnectionError:
...
attempts_remaining -= 1
time.sleep(0.5)
raise TimeoutError("No connection to the blueapi server")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
attempts_remaining = 20
while attempts_remaining > 0:
try:
client.get_environment()
return
except ConnectionError:
...
attempts_remaining -= 1
time.sleep(0.5)
raise TimeoutError("No connection to the blueapi server")
for _ in range(20):
try:
client.get_environment()
return
except ConnectionError:
...
time.sleep(0.5)
raise TimeoutError("No connection to the blueapi server")

nit: Why keep track when Python'll do it for ya mate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout, see also #763

Comment on lines +22 to +26
- name: Start RabbitMQ
uses: namoshek/rabbitmq-github-action@v1
with:
ports: "61613:61613"
plugins: rabbitmq_stomp
Copy link
Contributor

Choose a reason for hiding this comment

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

No longer running tests against ActiveMQ? Seems reasonable to me, given that the image is so deprecated, but we could replace it with Apache Artemis, just to make sure our Stomp interactions are generally valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We run the system tests of https://github.com/DiamondLightSource/bluesky-stomp/ against activemq and rabbitmq, which I think is sufficient

@callumforrester callumforrester merged commit cdfedb4 into main Dec 17, 2024
31 checks passed
@callumforrester callumforrester deleted the 630-system-tests-in-ci branch December 17, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run System Tests in CI
3 participants