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

Add CI job to run system tests #761

Merged
merged 7 commits into from
Dec 17, 2024
Merged

Add CI job to run system tests #761

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
06b439e
Add CI job to run system tests
callumforrester Dec 16, 2024
83fe567
Start blueapi server in background of system tests
callumforrester Dec 16, 2024
e7c276d
:Wait for server to start in system tests
callumforrester Dec 16, 2024
21bf8f1
Configure blueapi to talk to stomp
callumforrester Dec 16, 2024
5390068
Xfail auth system tests pending an issue
callumforrester Dec 17, 2024
cdc667d
Slightly neater wait-for-server function
callumforrester Dec 17, 2024
7071fed
Expand xfail message and simplify polling code
callumforrester Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions .github/workflows/_system_test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
on:
workflow_call:

env:
# https://github.com/pytest-dev/pytest/issues/2042
PY_IGNORE_IMPORTMISMATCH: "1"

jobs:
run:
runs-on: ubuntu-latest

steps:
- name: Checkout
uses: actions/checkout@v4
with:
# Need this to get version number from last tag
fetch-depth: 0

- name: Install python packages
uses: ./.github/actions/install_requirements

- name: Start RabbitMQ
uses: namoshek/rabbitmq-github-action@v1
with:
ports: "61613:61613"
plugins: rabbitmq_stomp
Comment on lines +22 to +26
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


- name: Start Blueapi Server
run: blueapi -c ${{ github.workspace }}/tests/unit_tests/example_yaml/valid_stomp_config.yaml serve &

- name: Run tests
run: tox -e system-test
3 changes: 0 additions & 3 deletions .github/workflows/_test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@ on:
env:
# https://github.com/pytest-dev/pytest/issues/2042
PY_IGNORE_IMPORTMISMATCH: "1"
BLUEAPI_TEST_STOMP_PORTS: "[61613,61614]"



jobs:
run:
Expand Down
5 changes: 5 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ jobs:
secrets:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}

system-test:
needs: check
if: needs.check.outputs.branch-pr == ''
uses: ./.github/workflows/_system_test.yml

container:
needs: check
if: needs.check.outputs.branch-pr == ''
Expand Down
21 changes: 20 additions & 1 deletion tests/system_tests/test_blueapi_system.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import pytest
from bluesky_stomp.models import BasicAuthentication
from pydantic import TypeAdapter
from requests.exceptions import ConnectionError

from blueapi.client.client import (
BlueapiClient,
Expand Down Expand Up @@ -43,7 +44,7 @@


@pytest.fixture
def client_without_auth(tmp_path) -> BlueapiClient:
def client_without_auth(tmp_path: Path) -> BlueapiClient:
return BlueapiClient.from_config(config=ApplicationConfig(auth_token_path=tmp_path))


Expand All @@ -58,6 +59,22 @@ def client_with_stomp() -> BlueapiClient:
)


@pytest.fixture(scope="module", autouse=True)
def wait_for_server():
client = BlueapiClient.from_config(config=ApplicationConfig())

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



# This client will have auth enabled if it finds cached valid token
@pytest.fixture
def client() -> BlueapiClient:
Expand Down Expand Up @@ -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.

def test_cannot_access_endpoints(
client_without_auth: BlueapiClient, blueapi_client_get_methods: list[str]
):
Expand All @@ -112,6 +130,7 @@ def test_cannot_access_endpoints(
getattr(client_without_auth, get_method)()


@pytest.mark.xfail(reason="https://github.com/DiamondLightSource/blueapi/issues/676")
def test_can_get_oidc_config_without_auth(client_without_auth: BlueapiClient):
assert client_without_auth.get_oidc_config() == OIDCConfig(
well_known_url="https://example.com/realms/master/.well-known/openid-configuration",
Expand Down
Loading