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

alexcottner · 2024-02-23T17:21:14Z

Resolves #565

Phase 1: (done, see this commit)

Run integration-tests and browser tests all together
Perform all tests through playwright or kinto clients
Update documentation

Phase 2: (done, see this commit)

Simplify test parameters
No longer allowed to skip test setup steps
Update documentation
Cleanup a bit

Phase 3: (to follow-up on)

Identify personas and their most common workflows
Expand tests to cover those

Phase 4: (to follow-up on)

Implement automated tests against stage (scheduled or after stage deployment)

Phase 5: (dreaming)

Automated releases to production

Open questions:

Did I simplify the test parameters enough?
- We're now able to define test users, but can no longer define parameters that wildly change test behavior.
- I'm hoping this makes testing against any environments very easy in the future via environment vars
Did I miss any documentation that needs to be updated?
Do we like/dislike any patterns in particular in here?

Post merge ToDo's:

Remove old integration-test image from dockerhub #569

…unning all tests together, light renaming/doc updates

…tion, cleaned up code a bit

alexcottner · 2024-02-27T16:49:45Z

.github/workflows/ci.yaml

@@ -93,51 +93,6 @@ jobs:
        run: make install
      - name: Run tests
        run: make test
-  integration_test:


Everything runs under browser-tests now

alexcottner · 2024-02-27T16:52:13Z

pyproject.toml

@@ -104,6 +104,7 @@ pytest-asyncio = "0.23.5"
 webtest = "3.0.0"
 playwright = "^1.41.2"
 pytest-playwright = "^0.4.4"
+nest_asyncio="^1.6.0"


Resolves an issue with playwright and the async tests from tests/plugins/test_signer.py

alexcottner · 2024-02-27T16:53:13Z

tests/conftest.py

 DEFAULT_SERVER = os.getenv("SERVER", "http://localhost:8888/v1")
 DEFAULT_SETUP_AUTH = os.getenv("SETUP_AUTH", "user:pass")
 DEFAULT_EDITOR_AUTH = os.getenv("EDITOR_AUTH", "editor:pass")
 DEFAULT_REVIEWER_AUTH = os.getenv("REVIEWER_AUTH", "reviewer:pass")
-DEFAULT_BUCKET = os.getenv("BUCKET", "main-workspace")


Reduced parameters to auth and mail directory to make things simpler

alexcottner · 2024-02-27T16:54:02Z

tests/plugins/test_attachment.py

@@ -0,0 +1,74 @@
+from playwright.sync_api import Browser


Moved from subfolder and removed fixtures that no longer exist

alexcottner · 2024-02-27T16:59:21Z

tests/utils.py

@@ -15,3 +16,10 @@ def upload_records(client: Client, num: int):
        record = client.create_record(data=data)
        records.append(record["data"])
    return records
+
+
+def create_extra_headers(username: str, password: str):


This is due to a quirk of playwright. If you give playwright credentials to hit an endpoint, it doesn't use them on the first request. It expects a 401 response to come back, and then it will retry with credentials.

This doesn't work for us because our endpoints are publicly available in many cases. So rather than returning 401 they'll return only the publicly available information instead.

This method allows us to force an auth header to be present when we need it.

leplatrem

This goes in the right direction 💯

There's something that could be better clarified: how do we articulate tests between local, dev, and stage?

We got rid of keep-existing, skip-setup, and review-enabled, I thought we would remplace it with the env name.

With a dedicated collection that we can hammer with these tests, we would have:

local: keep-existing=false, skip-setup=false, review-enabled=true
dev: keep-existing=false, skip-setup=true, review-enabled=false
stage: keep-existing=false, skip-setup=true, review-enabled=true

Which makes me wonder, when did we need to set keep-existing=true by default? @grahamalama do you remember?

leplatrem · 2024-02-28T11:22:01Z

README.rst


 .. code-block:: shell

    docker-compose build tests

-or download a pre-built container from `Dockerhub <https://hub.docker.com/r/mozilla/remote-settings-integration-tests>`_.
+or download a pre-built container from `Dockerhub <https://hub.docker.com/r/mozilla/remote-settings-browser-tests>`_.


Add a note to delete the mozilla/remote-settings-integration-tests image?

To clarify - do you mean add a note in here telling people to delete their old downloaded images? Or add a note for us to remove the old images from dockerhub?

for us to remove the old images from dockerhub

tests/plugins/test_signer.py

leplatrem · 2024-02-28T11:25:00Z

tests/run.sh

-    pytest browser_test.py --log-level=DEBUG --browser firefox
-    ;;
-*)
-    exec "$@"


Were we using this anywhere? (I personnally wasn't using it for local dev/testing)

Are we talking about run.sh in general? Or something else?

You removed the abitilty to use run.sh with abritary params/commands, that's why I was wondering if we, as developers debugging local containers or whatevers, were using this capability

Oh, the exec line specifically. I didn't fund any uses for it but I'm happy to put that back if we want to keep it just in case. If I wanted to run customized tests locally I'd just modify run.sh or just execute pytest myself, but maybe not everybody works that way.

Let's ask @grahamalama ! I'm fine with anything

tests/test_review.py

README.rst

tests/conftest.py

tests/plugins/test_signer.py

leplatrem · 2024-02-28T11:32:39Z

tests/plugins/test_signer.py

-    editor_client: RemoteSettingsClient,
-    reviewer_client: RemoteSettingsClient,
-    to_review_enabled: bool,
-):


This one was for env=="dev"

We have reviews enabled in dev currently and when running the docker compose images. Do we have any remote-settings environments with reviews disabled?

Review is disabled on dev. Better said, dual signoff is disabled. That mean you can approve your own changes.
That's the only environment with this setting.

Then in local containers, we have the optional testing.ini config, which has the flag set too:

remote-settings/config/testing.ini

Line 71 in 6169a51

kinto.signer.to_review_enabled = false

We can have this approach for our tests:

inspect the info on the root URL of the target server, to check whether review is disabled or not

hardcode somewhere that if env==dev, then review has to be disabled otherwise has to be enabled (this way we check that it's not accidentally enabled/disabled on one of the envs)

$ curl -s https://remote-settings-dev.allizom.org/v1/ | jq -r .capabilities.signer.to_review_enabled false $ curl -s https://remote-settings.allizom.org/v1/ | jq -r .capabilities.signer.to_review_enabled true

Bringing to_review_enabled back in next commit, but pulling it from server config in a fixture. Also leaving an easy path to pull additional config values down the road.

leplatrem · 2024-02-28T12:35:44Z

tests/conftest.py



 @pytest.fixture(scope="session")
 def source_collection(request) -> str:
-    return request.config.getoption("--collection")
+    return "browser-tests"


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?

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.

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.

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.

alexcottner · 2024-02-28T20:26:55Z

This goes in the right direction 💯

🥳 Nice

There's something that could be better clarified: how do we articulate tests between local, dev, and stage?

We got rid of keep-existing, skip-setup, and review-enabled, I thought we would remplace it with the env name.

Would the environment name relate to secrets in github or a 1password vault or something else? How would we pull those config values in?

With a dedicated collection that we can hammer with these tests, we would have:

* `local`: `keep-existing=false, skip-setup=false, review-enabled=true`

* `dev`: `keep-existing=false, skip-setup=true, review-enabled=false`

* `stage`: `keep-existing=false, skip-setup=true, review-enabled=true`

Do we still want these? I think it's simple enough to say "create the users and collection if they don't exist" in code. I have a separate question about review-enabled in another thread, but I don't know of any environments where it's disabled for us. I'd rather remove that if we can. Less branching more better.

leplatrem · 2024-02-29T10:12:08Z

Would the environment name relate to secrets in github or a 1password vault or something else? How would we pull those config values in?

The credentials will be put in GHA environment secrets I believe.

But I was just referring to the fact that instead of having flags with a lot of branching, we could pass the env name to the tests that behave differently in local, dev, or stage.
That being said, maybe there's a way to write these tests so that they don't on the env at all.

Do we still want these? I

No! (I was just providing a summary of their values on each env)

I think it's simple enough to say "create the users and collection if they don't exist" in code.

Yes, I think that works.

For safety reasons, when running the tests on dev and stage, we won't pass the credentials that are necessary to setup the server (SETUP_AUTH). So we just have to make sure the tests pass if empty/dummy setup credentials are provided when the server is already setup.

I have a separate question about review-enabled in another thread

#566 (comment)

leplatrem · 2024-02-29T10:18:39Z

.github/workflows/ci.yaml

@@ -177,7 +132,7 @@ jobs:
        with:
          cache-from: type=local,src=${{ env.DOCKER_CACHE}}/tests
          cache-to: type=local,dest=${{ env.DOCKER_CACHE}}/tests,mode=max
-          file: IntegrationTests.Dockerfile
+          file: BrowserTests.Dockerfile
          context: .

      - name: Run browser tests


Why are the logs so long for this step? Could we make sure we don't have to scroll that much in the workflows step logs before seeing the tests results?

They're the same warnings as before I think. See this previous integration-tests run and this previous browser-tests run. Five DeprecationWarnings from autograph_utils, one from kinto_http, and a config warning about sensitive_url.

I could suppress the warnings but that seems like a bad idea. We should probably get updating autograph and kinto_http on the radar.

Added -q to docker compose build to quiet that part down at least.

leplatrem

🎉

Just tiny details left!

Good job Alex, this is slick 💯

leplatrem · 2024-03-01T16:10:08Z

tests/plugins/test_signer.py

 ):
    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"})


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

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

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

leplatrem · 2024-03-01T16:12:09Z

tests/test_endpoints.py

@@ -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"]


do we want to add

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

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

alexcottner added the python Pull requests that update Python code label Feb 23, 2024

alexcottner added 3 commits February 23, 2024 10:57

Phase 1 test changes - merging integration tests and browser tests, r…

67b7ee7

…unning all tests together, light renaming/doc updates

Phase 2 test changes - simplifying test parameters, updated documenta…

fbe068d

…tion, cleaned up code a bit

fixing merge conflict

d52851b

alexcottner commented Feb 27, 2024

View reviewed changes

Removing unnecesary shift from run.sh

36170f5

alexcottner marked this pull request as ready for review February 27, 2024 17:04

alexcottner requested a review from a team as a code owner February 27, 2024 17:04

alexcottner changed the title ~~WIP - Draft - Ignore - Getting browser tests ready~~ Browser-tests and integration-tests merge, paving trail for better gatekeeper tests Feb 27, 2024

alexcottner marked this pull request as draft February 27, 2024 17:04

alexcottner marked this pull request as ready for review February 27, 2024 17:05

leplatrem reviewed Feb 28, 2024

View reviewed changes

alexcottner added 2 commits February 28, 2024 13:28

First round of feedback fixes

6855304

adding another comment and running format

1ccdac4

leplatrem reviewed Feb 29, 2024

View reviewed changes

alexcottner mentioned this pull request Feb 29, 2024

Remove old integration-test image from dockerhub #569

Open

alexcottner added 4 commits February 29, 2024 12:35

Adding -q to browser-test docker compose build

ec85b13

Adding to_review_enabled back, but pulling it from server config

83880c0

reverting collection name

d7fd9bc

handling no setup_auth

8aa71ad

leplatrem approved these changes Mar 1, 2024

View reviewed changes

Tweaks from latest feedback

6d59557

alexcottner requested a review from grahamalama March 1, 2024 16:53

fixing merge conflict

f1d8a76

alexcottner merged commit b80ab04 into mozilla:main Mar 4, 2024
4 checks passed

alexcottner deleted the playwright-extended branch March 4, 2024 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alexcottner commented Feb 23, 2024 •

edited

Loading

alexcottner Feb 27, 2024

alexcottner Feb 27, 2024

alexcottner Feb 27, 2024

alexcottner Feb 27, 2024

alexcottner Feb 27, 2024

leplatrem left a comment

leplatrem Feb 28, 2024

alexcottner Feb 28, 2024

leplatrem Feb 29, 2024

leplatrem Feb 28, 2024

alexcottner Feb 28, 2024

leplatrem Feb 29, 2024

alexcottner Feb 29, 2024

leplatrem Mar 1, 2024

leplatrem Feb 28, 2024

alexcottner Feb 28, 2024

leplatrem Feb 29, 2024 •

edited

Loading

alexcottner Feb 29, 2024

leplatrem Feb 28, 2024

alexcottner Feb 28, 2024

leplatrem Feb 29, 2024

alexcottner Feb 29, 2024

alexcottner commented Feb 28, 2024

leplatrem commented Feb 29, 2024

leplatrem Feb 29, 2024

alexcottner Feb 29, 2024

alexcottner Feb 29, 2024

leplatrem left a comment

leplatrem Mar 1, 2024

alexcottner Mar 1, 2024

leplatrem Mar 1, 2024

alexcottner Mar 1, 2024

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

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

Conversation

alexcottner commented Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leplatrem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leplatrem Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcottner commented Feb 28, 2024

leplatrem commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leplatrem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcottner commented Feb 23, 2024 •

edited

Loading

leplatrem Feb 29, 2024 •

edited

Loading