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 e2e tests to remaining services #242

Merged
merged 12 commits into from
Aug 3, 2022
Merged

Add e2e tests to remaining services #242

merged 12 commits into from
Aug 3, 2022

Conversation

mic-max
Copy link
Contributor

@mic-max mic-max commented Jul 28, 2022

Fixes #195

Changes

  • Add CartService e2e Tests
  • Product Service e2e Tests
  • Shipping Service e2e Tests
  • Currency Service e2e Tests
  • Add e2e tests to Checkout Service + boilerplate for email service
  • Add e2e tests to Ad Service

image

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes

@julianocosta89
Copy link
Member

hey @mic-max do we really need to expose the localhost ports?
I've done the same question to @xoscar here: #236 (comment)

For testing instead of opening the localhost, we could spin up a container within the same docker network and execute the test.
The test container would be able to call the other containers by name:port, and we wouldn't need to worry about modifying all the ports.

Another solution would be to keep the internal ports the apps are already using, and expose a different one within the localhost.

-p 8080:80 | Map TCP port 80 in the container to port 8080 on the Docker host.

In that case we could have multiple container running on port 5050 (for instance), and just change the localhost exposed port.

I'd rather go for the 1st solution.

@mic-max
Copy link
Contributor Author

mic-max commented Jul 28, 2022

@julianocosta89 Is there any downside to opening to ports to the localhost? I think it is the simplest way to do it.

The test container option sounds doable, does anyone know if that's a common architecture for testing docker containers?

@julianocosta89
Copy link
Member

@julianocosta89 Is there any downside to opening to ports to the localhost? I think it is the simplest way to do it.

No actual downside in a demo perspective, just in a real life app.
The main issue now is that we have 2 PRs changing port numbers.
Someone will have to solve some merging conflicts 😅 .

Another point is that we need to ensure all services all communicating with each other with the new ports. But that is fine, considering that it will be done once.

@mic-max mic-max marked this pull request as ready for review July 28, 2022 20:10
@mic-max mic-max requested a review from a team July 28, 2022 20:10
@cartersocha
Copy link
Contributor

@fatsheep9146, @julianocosta89 any further comments / suggestions ?

@fatsheep9146
Copy link
Contributor

@fatsheep9146, @julianocosta89 any further comments / suggestions ?

LGTM

@cartersocha
Copy link
Contributor

@mic-max please resolve the conflicts then we'll merge

@mic-max
Copy link
Contributor Author

mic-max commented Aug 3, 2022

@mic-max please resolve the conflicts then we'll merge

Resolved 😊

@cartersocha cartersocha merged commit 82c295e into open-telemetry:main Aug 3, 2022
@mic-max mic-max deleted the more-e2e-tests branch August 3, 2022 06:05
cartersocha added a commit that referenced this pull request Aug 3, 2022
This is a follow-up to #248, which in turn was resolving: #242 (comment)

The ruby app is relying on convention, and by convention a `PORT` env
varable will determine which port the `puma` webserver actually listens
on. So - we don't need to set it explicitly in the compose file, but
that means we need to set it elsewhere somehow.

I tried a few hacks with the dockerfile:
- Trying to pass `PORT=$EMAIL_SERVICE_PORT`
- Trying to pass `-p $EMAIL_SERVICE_PORT` in the command

But those didn't work and I gave up. So instead we just set it directly
in the sinatra app ourselves. (Please don't ask me how that actually
gets set down all the way through sinatra -> rack -> puma, because
truthfully I do not know: rack-based servers are basically magic).

We can see that it works:

```
@ahayworth ➜ /workspaces/opentelemetry-demo-webstore (ahayworth/emailservice-port-things ✗) $ git grep EMAIL_SERVICE_P
ORT
.env:EMAIL_SERVICE_PORT=6060
```

```
@ahayworth ➜ /workspaces/opentelemetry-demo-webstore (ahayworth/emailservice-port-things ✗) $ docker-compose up --build emailservice
email-service            | == Sinatra (v2.2.0) has taken the stage on 6060 for production with backup from Puma
email-service            | I, [2022-08-03T13:08:27.643291 #1]  INFO -- : Instrumentation: OpenTelemetry::Instrumentation::Sinatra was successfully installed with the following options {}
email-service            | Puma starting in single mode...
email-service            | * Puma version: 5.6.4 (ruby 3.1.2-p20) ("Birdie's Version")
email-service            | *  Min threads: 0
email-service            | *  Max threads: 5
email-service            | *  Environment: production
email-service            | *          PID: 1
email-service            | * Listening on http://0.0.0.0:6060
email-service            | Use Ctrl-C to stop
```

Co-authored-by: Carter Socha <43380952+cartersocha@users.noreply.github.com>
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
This is a follow-up to open-telemetry#248, which in turn was resolving: open-telemetry#242 (comment)

The ruby app is relying on convention, and by convention a `PORT` env
varable will determine which port the `puma` webserver actually listens
on. So - we don't need to set it explicitly in the compose file, but
that means we need to set it elsewhere somehow.

I tried a few hacks with the dockerfile:
- Trying to pass `PORT=$EMAIL_SERVICE_PORT`
- Trying to pass `-p $EMAIL_SERVICE_PORT` in the command

But those didn't work and I gave up. So instead we just set it directly
in the sinatra app ourselves. (Please don't ask me how that actually
gets set down all the way through sinatra -> rack -> puma, because
truthfully I do not know: rack-based servers are basically magic).

We can see that it works:

```
@ahayworth ➜ /workspaces/opentelemetry-demo-webstore (ahayworth/emailservice-port-things ✗) $ git grep EMAIL_SERVICE_P
ORT
.env:EMAIL_SERVICE_PORT=6060
```

```
@ahayworth ➜ /workspaces/opentelemetry-demo-webstore (ahayworth/emailservice-port-things ✗) $ docker-compose up --build emailservice
email-service            | == Sinatra (v2.2.0) has taken the stage on 6060 for production with backup from Puma
email-service            | I, [2022-08-03T13:08:27.643291 open-telemetry#1]  INFO -- : Instrumentation: OpenTelemetry::Instrumentation::Sinatra was successfully installed with the following options {}
email-service            | Puma starting in single mode...
email-service            | * Puma version: 5.6.4 (ruby 3.1.2-p20) ("Birdie's Version")
email-service            | *  Min threads: 0
email-service            | *  Max threads: 5
email-service            | *  Environment: production
email-service            | *          PID: 1
email-service            | * Listening on http://0.0.0.0:6060
email-service            | Use Ctrl-C to stop
```

Co-authored-by: Carter Socha <43380952+cartersocha@users.noreply.github.com>
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.

Add E2E service tests
4 participants