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

fix e2e test #336

Merged
merged 27 commits into from
Jun 20, 2024
Merged

fix e2e test #336

merged 27 commits into from
Jun 20, 2024

Conversation

da13da
Copy link
Contributor

@da13da da13da commented Jun 17, 2024

Fixed problem with e2e test crashing

@@ -143,3 +141,8 @@ runs:
shell: bash
run: docker compose -f test_resource/compose.yaml logs
if: ${{ always() }}

- name: (run) show colima log
Copy link
Contributor Author

@da13da da13da Jun 18, 2024

Choose a reason for hiding this comment

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

Random drops in 'colima', so I'll add it to check the log in case of future fail.

@@ -37,7 +37,6 @@ runs:
pid=$(docker exec $container_id pgrep -f nodex-agent)
docker exec $container_id kill -SIGINT $pid
sleep 3
if: ${{ always() }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove sigint test because it is not necessary to add "always"

platform: linux/amd64
volumes:
- ./did_sidetree.yaml:/tmp/api.yaml:ro
ports:
- "4010:4010"
healthcheck:
# this docker image doens't have curl
test: wget --no-verbose --tries=1 --spider http://localhost:4010/health || exit 1
test: ["CMD-SHELL", "curl -f http://localhost:4010/health || exit 1"]
Copy link
Contributor Author

@da13da da13da Jun 18, 2024

Choose a reason for hiding this comment

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

It passes healthcheck for curl, but fails after several attempts with wget. I don't know why.
I have confirmed that prism is running fine even when wget fails.

@@ -138,6 +138,31 @@ paths:
schema:
$ref: "#/components/schemas/Error"

/v1/metric:
Copy link
Contributor Author

@da13da da13da Jun 18, 2024

Choose a reason for hiding this comment

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

Added because there are many unnecessary request failure logs because the metric schema does not exist.

@da13da da13da marked this pull request as ready for review June 18, 2024 14:14
@da13da da13da requested a review from a team as a code owner June 18, 2024 14:14
@da13da
Copy link
Contributor Author

da13da commented Jun 18, 2024

I checked the resources for this issue and everything seemed fine.
Also, one of the multiple requests thrown by metric left a failure log, so there is still a possibility that e2e parallelization or something else is involved, causing the network to disconnect.

Copy link
Contributor

@c0ridrew c0ridrew left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, commented!

.github/actions/e2e/action.yml Outdated Show resolved Hide resolved
test_resource/compose.yaml Outdated Show resolved Hide resolved
Comment on lines 28 to 29
# this docker image doens't have curl
test: wget --no-verbose --tries=1 --spider http://localhost:4010/health || exit 1
test: ["CMD-SHELL", "curl -f http://localhost:4010/health || exit 1"]
Copy link
Contributor

@c0ridrew c0ridrew Jun 19, 2024

Choose a reason for hiding this comment

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

Same here, # this docker image doens't have curl you can delete this comment if curl is installed manually ?

@da13da da13da mentioned this pull request Jun 19, 2024
@c0ridrew c0ridrew self-requested a review June 19, 2024 23:39
Copy link
Contributor

@c0ridrew c0ridrew left a comment

Choose a reason for hiding this comment

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

LGTM!

@da13da da13da merged commit 63a2db9 into main Jun 20, 2024
12 checks passed
@da13da da13da deleted the fix/e2e branch June 20, 2024 04:07
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