E2E Tests version 1.0 #731

iwphonedo · 2025-01-07T18:33:37Z

This comprehensive commit includes major improvements and enhancements to the OPAL development environment, focusing on debugging, authorization, containerized testing, and project structure optimization. Key highlights include:
1. OPAL Development Enhancements: • Introduced remote debugging capabilities using debugpy and enhanced logging across various components. • Improved OPAL server and client configuration with better container integration and dynamic port management. • Enabled Gitea for local policy management, replacing GitHub for enhanced control and security in testing environments.
2. Refactoring and Streamlining: • Refactored the codebase to improve readability, remove deprecated files, and enhance test fixtures. • Reorganized project structure and consolidated environment setup for consistency. • Simplified session and container management for test execution using pytest and testcontainers.
3. Testing and Configuration Improvements: • Enhanced test automation with new fixtures, logging updates, and RBAC policy improvements. • Improved Docker image handling, session matrix parameterization, and environment variable management for better efficiency and clarity. • Added global exception handling to improve error reporting during tests.
4. Documentation and Cleanup: • Updated README documentation to reflect changes in test organization and container setup. • Removed unused files and components to streamline the project structure.

Fixes Issue

Changes proposed

Check List (Check all the applicable boxes)

I sign off on contributing this submission to open-source
My code follows the code style of this project.
My change requires changes to the documentation.
I have updated the documentation accordingly.
All new and existing tests passed.
This PR does not contain plagiarized content.
The title of my pull request is a short description of the requested changes.

Screenshots

Note to reviewers

This comprehensive commit includes major improvements and enhancements to the OPAL development environment, focusing on debugging, authorization, containerized testing, and project structure optimization. Key highlights include: 1. OPAL Development Enhancements: • Introduced remote debugging capabilities using debugpy and enhanced logging across various components. • Improved OPAL server and client configuration with better container integration and dynamic port management. • Enabled Gitea for local policy management, replacing GitHub for enhanced control and security in testing environments. 2. Refactoring and Streamlining: • Refactored the codebase to improve readability, remove deprecated files, and enhance test fixtures. • Reorganized project structure and consolidated environment setup for consistency. • Simplified session and container management for test execution using pytest and testcontainers. 3. Testing and Configuration Improvements: • Enhanced test automation with new fixtures, logging updates, and RBAC policy improvements. • Improved Docker image handling, session matrix parameterization, and environment variable management for better efficiency and clarity. • Added global exception handling to improve error reporting during tests. 4. Documentation and Cleanup: • Updated README documentation to reflect changes in test organization and container setup. • Removed unused files and components to streamline the project structure.

netlify · 2025-01-07T18:34:08Z

✅ Deploy Preview for opal-docs canceled.

Name	Link
🔨 Latest commit	`59b3404`
🔍 Latest deploy log	https://app.netlify.com/sites/opal-docs/deploys/677ee1ee6e573e0008ed6af7

packages/opal-client/opal_client/main.py

packages/opal-client/requires.txt

danyi1212 · 2025-01-08T13:06:21Z

packages/opal-server/opal_server/data/api.py

@@ -86,6 +87,8 @@ async def get_data_sources_config(authorization: Optional[str] = Header(None)):
        token = get_token_from_header(authorization)
        if data_sources_config.config is not None:
            logger.info("Serving source configuration")
+            logger.info("Source config: {config}", config=data_sources_config.config)


Is this intentional?

@danyi1212 Can be removed. If needed will be submitted on its own PR.

danyi1212 · 2025-01-08T13:06:47Z

packages/opal-server/opal_server/main.py

@@ -1,3 +1,9 @@
+
+import debugpy


Is this intentional?

Supported now in the dockerfile for local debugging

danyi1212 · 2025-01-08T13:10:56Z

tests/README.md

+Got it! If you'd like, I can incorporate code snippets or provide clarifications. Just let me know what specific details or examples you'd like me to include. Here's the README file in Markdown format:
+
+---


Looks like an incorrect copy-paste from ChatGPT 🙂

Suggested change

Got it! If you'd like, I can incorporate code snippets or provide clarifications. Just let me know what specific details or examples you'd like me to include. Here's the README file in Markdown format:

---

danyi1212 · 2025-01-08T13:40:40Z

tests/containers/settings/postgres_broadcast_settings.py

+from testcontainers.core.utils import setup_logger
+
+
+class PostgresBroadcastSettings:


I don't like the settings being decoupled from the container class itself, it's harder to navigate and read.
I would recommend moving the settings to right before the container implementation, same with any other settings implementation.
Also, I think Config is a better term in this context, but I agree with either :)

danyi1212 · 2025-01-08T13:42:00Z

tests/docker/Dockerfile.client

@@ -0,0 +1,17 @@
+FROM permitio/opal-client:latest


This should be using a locally built image from the current source code, rather than loading a pre-built image from Docker Hub / Desktop (applied for all Dockerfiles here)

@danyi1212 this and Dockerfile.server are just to support debugging of the opal image. but aren't based on the local code. The local version is in .local dockers. if you think this is not necessary, let me know and I will remove these 2 dockerfiles.

danyi1212 · 2025-01-08T13:51:40Z

tests/fixtures/broadcasters.py

+    unless an exception is raised during teardown.
+    """
+    try:
+        with PostgresBroadcastContainer(


There are a lot of configurations that may need to be set for each of those objects. Putting them inside fixture might be easy to write, but it hides the complexities inside deep, hard to follow fixtures.

Configuring the containers for each test case will require setting multiple fixtures or global variables (or even environment variables) making it very hard to follow and requires a lot of back and forth to the docs to understand or to write.

I would instead use the containers directly inside the tests logs, making it possible to pass any required configurations directly to the container init. For example, a test setup that requires an OPAL Server, 2 OPAL Clients, Gitea as Policy Store and a Postrges as Broadcaster might look like:

postgres_broadcaster = PostgresContainer() gitea_store = GiteaContainer() opal_server = OpalServerContainer(broadcaster=postgres_broadcaster, policy_store=gitea_store) opal_client_1 = OpalClientContainer(server_url=server.url) opal_client_2 = OpalClientContainer(server_url=server.url) with (postgres_broadcaster, opal_sever, opal_client_1, opal_client_2): # Test logic here...

You are correct about the need for multiple fixtures, but this is what they are designed for. Putting the setup code inside the test, will prevent us from running sets of tests cases on different setups. So if you want to test scenario1 through N on different setups, you would extract the setup to an outside function and pass it to the tests, that's we do here.

But, in order to somewhat simplify the complexity of the fixtures we added the session_matrix class. It allows to create matrix of setups and then run sets of tests on that setup.

danyi1212 · 2025-01-08T13:56:55Z

tests/containers/opal_client_container.py

+from tests.containers.settings.opal_client_settings import OpalClientSettings
+
+
+class OpalClientContainer(PermitContainer, DockerContainer):


Using multiple inheritance and initializing both parents inside the __init__ is a very complex Python setup that is hard to understand.
I understand it is used for "mixin"-like inheritance, but it would be simpler to have a single base class inherit from DockerContainer then have everything inherited from it

I was looking for mixin, because inheritanc won't work as some containers are already inheriting from DockerContainer. E.g. RedisContainer, PostgresContainer, NginxContainer, KafkaContainer. So mixin was the only sort of OOP approach.

danyi1212 · 2025-01-08T14:08:25Z

tests/containers/settings/opal_client_settings.py

+
+    def getEnvVars(self):
+        env_vars = {
+            "OPAL_SERVER_URL": self.opal_server_url,


You could have used pydantic-settings for this.

Saying that, I don't think we need to be able to configure every little configuration for the tests from environment variables. This would probably just break tests, as they need very specific intentional configurations for each service container.

I would use plain PyDantic models or dataclasses for any complex configurations needed to be passed to Container instances on initiation for autocomplete and self-documenting, easy to read code.

danyi1212 · 2025-01-08T15:52:41Z

tests/utils.py

@@ -57,8 +57,13 @@ def build_docker_image(docker_file: str, image_name: str, session_matrix: dict):
            image = docker_client.images.get(image_name)

    if not image:
-        # context_path=os.path.join(os.path.dirname(__file__), ".."),  # Expands the context
-        context_path = ".."
+        if "tests" in os.path.abspath(__file__):


Better to use pathlib instead, as its clearer and more modern

iwphonedo marked this pull request as ready for review January 7, 2025 18:33

danyi1212 self-requested a review January 8, 2025 12:48

danyi1212 assigned iwphonedo Jan 8, 2025

danyi1212 reviewed Jan 8, 2025

View reviewed changes

Refactor test scripts and improve documentation comments

a3f892a

danyi1212 reviewed Jan 8, 2025

View reviewed changes

iwphonedo added 2 commits January 8, 2025 22:36

Refactor service configurations and update container dependencies

ed3c033

Add end-to-end tests to GitHub Actions workflow

59b3404

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

E2E Tests version 1.0 #731

E2E Tests version 1.0 #731

iwphonedo commented Jan 7, 2025

netlify bot commented Jan 7, 2025 •

edited

Loading

danyi1212 Jan 8, 2025

iwphonedo Jan 8, 2025

danyi1212 Jan 8, 2025

iwphonedo Jan 8, 2025

danyi1212 Jan 8, 2025

danyi1212 Jan 8, 2025

danyi1212 Jan 8, 2025

iwphonedo Jan 8, 2025

danyi1212 Jan 8, 2025

iwphonedo Jan 8, 2025

danyi1212 Jan 8, 2025

iwphonedo Jan 8, 2025

danyi1212 Jan 8, 2025

danyi1212 Jan 8, 2025

		Got it! If you'd like, I can incorporate code snippets or provide clarifications. Just let me know what specific details or examples you'd like me to include. Here's the README file in Markdown format:

		---

		from testcontainers.core.utils import setup_logger


		class PostgresBroadcastSettings:

		from tests.containers.settings.opal_client_settings import OpalClientSettings


		class OpalClientContainer(PermitContainer, DockerContainer):

E2E Tests version 1.0 #731

Are you sure you want to change the base?

E2E Tests version 1.0 #731

Conversation

iwphonedo commented Jan 7, 2025

Fixes Issue

Changes proposed

Check List (Check all the applicable boxes)

Screenshots

Note to reviewers

netlify bot commented Jan 7, 2025 • edited Loading

✅ Deploy Preview for opal-docs canceled.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Jan 7, 2025 •

edited

Loading