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

Browser-tests and integration-tests merge, paving trail for better gatekeeper tests #566

Merged
merged 12 commits into from
Mar 4, 2024
11 changes: 11 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@ def source_collection(request) -> str:
return "browser-tests"
Copy link
Contributor

Choose a reason for hiding this comment

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

This collection has to be created and configured (ie. be put here)

We already have one integration-tests. It could be a little bit tedious to recreate a new collection and the related users (currently integration-test, integration-test-editor, integration-test-reviewer).
How bad would you feel to keep the integration tests name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping the collection name integration-tests doesn't bother me too much. I was just trying to keep our language consistent.

My first choice was to create bucket and a collection for each test module, then delete the bucket at the end. After looking at how we manage the kinto buckets/collections in remote settings that seemed problematic.

Copy link
Contributor

Choose a reason for hiding this comment

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

After looking at how we manage the kinto buckets/collections in remote settings that seemed problematic.

Yes, on stage we don't want to allow creations of arbitrary buckets/collections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added logic to handle no setup_auth in tests. I think this ties up the current loose ends, will circle back with fresh eyes in the morning.



@pytest.fixture(scope="session")
def server_config(browser, server) -> dict:
resp = browser.new_context().request.get(server)
return resp.json()


@pytest.fixture(scope="session")
def to_review_enabled(server_config) -> bool:
return server_config["capabilities"]["signer"]["to_review_enabled"]


@pytest.fixture(scope="session")
def mail_dir(request) -> str:
directory = request.config.getoption("--mail-dir")
Expand Down
52 changes: 51 additions & 1 deletion tests/plugins/test_signer.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,11 @@ def test_signer_plugin_capabilities(anonymous_client: RemoteSettingsClient):
async def test_signer_plugin_full_workflow(
editor_client: RemoteSettingsClient,
reviewer_client: RemoteSettingsClient,
to_review_enabled: bool,
):
if not to_review_enabled:
pytest.skip("to-review disabled")

resource = signed_resource(editor_client)

dest_col = resource["destination"].get("collection")
Expand Down Expand Up @@ -158,6 +162,38 @@ async def test_signer_plugin_full_workflow(
raise


@pytest.mark.asyncio()
async def test_workflow_without_review(
editor_client: RemoteSettingsClient,
reviewer_client: RemoteSettingsClient,
to_review_enabled: bool,
):
if to_review_enabled:
pytest.skip("to-review enabled")

resource = signed_resource(editor_client)

dest_col = resource["destination"].get("collection")
dest_client = editor_client.clone(
bucket=resource["destination"]["bucket"],
collection=dest_col or editor_client.collection_name,
auth=tuple(),
)

upload_records(editor_client, 1)
reviewer_client.patch_collection(data={"status": "to-sign"})
changeset = dest_client.fetch_changeset()
records = changeset["changes"]
timestamp = changeset["timestamp"]
signature = changeset["metadata"]["signature"]

try:
await verify_signature(records, timestamp, signature)
except Exception:
print("Signature KO")
raise


def test_signer_plugin_rollback(
editor_client: RemoteSettingsClient,
):
Expand All @@ -176,13 +212,15 @@ def test_signer_plugin_rollback(
def test_signer_plugin_refresh(
editor_client: RemoteSettingsClient,
reviewer_client: RemoteSettingsClient,
to_review_enabled: bool,
):
resource = signed_resource(editor_client)
preview_bucket = resource["preview"]["bucket"]
dest_bucket = resource["destination"]["bucket"]
upload_records(editor_client, 5)

editor_client.patch_collection(data={"status": "to-review"})
if to_review_enabled:
editor_client.patch_collection(data={"status": "to-review"})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can execute this always, since it's not blocked when review is disabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For whatever reason, that's how the old test worked. link

But yeah I can remove it since it's harmless.


reviewer_client.patch_collection(data={"status": "to-sign"})
signature_preview_before = (editor_client.get_collection(bucket=preview_bucket))[
Expand All @@ -207,7 +245,11 @@ def test_signer_plugin_refresh(
def test_cannot_skip_to_review(
leplatrem marked this conversation as resolved.
Show resolved Hide resolved
editor_client: RemoteSettingsClient,
reviewer_client: RemoteSettingsClient,
to_review_enabled: bool,
):
if not to_review_enabled:
pytest.skip("to-review disabled")

upload_records(editor_client, 1)

with pytest.raises(KintoException):
Expand All @@ -218,7 +260,11 @@ def test_same_editor_cannot_review(
setup_client: RemoteSettingsClient,
editor_client: RemoteSettingsClient,
reviewer_client: RemoteSettingsClient,
to_review_enabled: bool,
):
if not to_review_enabled:
pytest.skip("to-review disabled")

# Add reviewer to editors, and vice-versa.
reviewer_id = (reviewer_client.server_info())["user"]["id"]
data = JSONPatch([{"op": "add", "path": "/data/members/0", "value": reviewer_id}])
Expand All @@ -240,7 +286,11 @@ def test_rereview_after_cancel(
setup_client: RemoteSettingsClient,
editor_client: RemoteSettingsClient,
reviewer_client: RemoteSettingsClient,
to_review_enabled: bool,
):
if not to_review_enabled:
pytest.skip("to-review disabled")

# Add reviewer to editors, and vice-versa.
reviewer_id = (reviewer_client.server_info())["user"]["id"]
data = JSONPatch([{"op": "add", "path": "/data/members/0", "value": reviewer_id}])
Expand Down
7 changes: 7 additions & 0 deletions tests/test_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ def test_heartbeat(server: str, context: BrowserContext):
assert resp.status == 200


def test_config(server_config, to_review_enabled):
assert server_config["project_name"]
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to add

assert server_config["to_review_enabled"] == "dev" not in server_config["project_name"].lower():

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that seems reasonable. I think I'll need some parenthesis though

assert server_config["project_version"]
assert server_config["http_api_version"]
assert server_config["url"]


def test_permissions_endpoint(
server: str,
editor_auth: Auth,
Expand Down
Loading