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 Makefile to Automate Local E2E Test Setup #50

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aruokhai
Copy link
Contributor

Objective
Introduce a Makefile to streamline the local setup and execution of End-to-End (E2E) tests, improving developer efficiency.

Problem
Running E2E tests requires specific processes to be set up before execution, and manually configuring these processes for local testing can be time-consuming and error-prone. This repetitive task reduces developer productivity and can lead to inconsistencies in the testing environment. Automating this setup with a Makefile will make local testing more efficient and reliable.

Changes

  • Added a Makefile to automate the setup and execution of E2E tests locally.
  • Added a README with clear instructions on how to run the tests using the new setup.

Scope of Change
This is a non-critical update, as it only affects the local environment for running E2E tests and does not impact production code.

Copy link
Collaborator

@theanmolsharma theanmolsharma left a comment

Choose a reason for hiding this comment

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

  1. I want you to look into if we absolutely need pm2, I think we should be able to achieve something similar without it.
  2. We need a command or something to view the indexer logs during/after the run. We should be able to use tail command to achieve this.

e2e/makefile Outdated Show resolved Hide resolved
e2e/README.md Outdated
Comment on lines 23 to 31
## Step 2: Install `PM2` Globally

To install PM2 globally, follow these steps:

1. **Open your terminal** (Command Prompt, PowerShell, or Terminal on macOS/Linux).

2. **Run the following command** to install PM2 globally using npm:

```bash
npm install -g pm2
```

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like that the user needs to install a global dependency. We should add it as a dev dependency and do npx pm2

e2e/makefile Outdated
# Stop and remove Docker Compose services
down:
@echo "Stopping and removing Docker Compose services..."
docker compose -f $(DOCKER_COMPOSE_FILE) down
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
docker compose -f $(DOCKER_COMPOSE_FILE) down
docker compose -f $(DOCKER_COMPOSE_FILE) down -v

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The -v option is meant to remove named volumes, which we do not use.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It also removes anonymous volumes which are used in the image.

e2e/makefile Outdated
Comment on lines 53 to 54
clean: down
@echo "Clean up completed."
Copy link
Collaborator

Choose a reason for hiding this comment

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

redundant with down, please remove

@aruokhai
Copy link
Contributor Author

The problem with using '&' to start background tasks is that they must be terminated manually. In contrast, PM2 simplifies this process by assigning an identifier for easy management and control

@aruokhai
Copy link
Contributor Author

@theanmolsharma, Using the tail command has also been challenging due to the issues with background PIDs. As an alternative, I opted to use output log files.

added makerfile to make local e2e test a breeze:

working

improved maker file structure with output logs for debug purpose
Comment on lines +39 to +41
#e2e
/.logs
Copy link
Collaborator

Choose a reason for hiding this comment

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

not required because we already have *.log on L7

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to keep it still, it should be /e2e/.logs/ not /.logs/

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.

2 participants