Skip to content
This repository has been archived by the owner on Apr 24, 2024. It is now read-only.

E2E: Improve documentation/usage #784

Merged
merged 6 commits into from
Aug 6, 2023

Conversation

4ydan
Copy link
Contributor

@4ydan 4ydan commented Aug 4, 2023

Closes #783
Closes #782

Basics

  • I added a line to /doc/CHANGELOG.md
  • The PR is rebased with current master.
  • Details of what you changed are in commit messages.
  • References to issues, e.g. close #X, are in the commit messages.
  • The buildserver is happy.

Checklist

  • I have installed and I am using pre-commit hooks
  • I fully described what my PR does in the documentation
    (not in the PR description)
  • I fixed all affected documentation
  • I fixed the introduction tour
  • I wrote migrations in a way that they are compatible with already present data
  • I fixed all affected decisions
  • I added unit tests for my code
  • I added code comments, logging, and assertions as appropriate
  • I translated all strings visible to the user
  • I mentioned every code or binary not directly written or done by me in reuse syntax
  • I created left-over issues for things that are still to be done
  • Code is conforming to our Architecture
  • Code is conforming to our Guidelines
    (exceptions are documented)
  • Code is consistent to our Design Decisions

Review

  • I've tested the code
  • I've read through the whole code
  • Examples are well chosen and understandable

@4ydan 4ydan marked this pull request as draft August 4, 2023 12:26
@4ydan
Copy link
Contributor Author

4ydan commented Aug 4, 2023

May I allow
image
@markus2330 ?
It seems to have a problem with executing a shell script as entrypoint for the e2e docker image.

The real complication here is that I need to delete all maps from the database after a local test run and I dont have to do that in CI. So I get these two different use cases which I both need to handle somehow.

Also the entrypoint.sh is used from the top level makefile. So I really dont want to duplicate any lines of code here.

@4ydan 4ydan force-pushed the documentation/783-improve-e2e-docu branch from 97d0deb to 453a265 Compare August 4, 2023 14:53
@4ydan 4ydan marked this pull request as ready for review August 4, 2023 15:26
@markus2330
Copy link
Contributor

It seems to have a problem with executing a shell script as entrypoint for the e2e docker image.

Which line of code has this problem? It is quite clearly indicated that something like this shouldn't be done.

The real complication here is that I need to delete all maps from the database after a local test run and I dont have to do that in CI. So I get these two different use cases which I both need to handle somehow.

Why would you need to delete all maps? You can simply require that E2E are only executed on an empty database.

Also the entrypoint.sh is used from the top level makefile. So I really dont want to duplicate any lines of code here.

Yes, I agree.

@4ydan
Copy link
Contributor Author

4ydan commented Aug 4, 2023

Which line of code has this problem?

I cant clearly tell. I would guess it doesnt like the shell script invocation inside the dockerfile.

It is quite clearly indicated that something like this shouldn't be done.

Yes I agree, I was just not sure how "dangerous" this really is. I will try to find another way, just like the other scripts maybe.
I thought it would have been nice to have a docker entrypoint.

Why would you need to delete all maps? You can simply require that E2E are only executed on an empty database.

You need to do that everytime you rerun the e2e tests otherwise you will get wrong test results.

@markus2330
Copy link
Contributor

markus2330 commented Aug 4, 2023

You need to do that everytime you rerun the e2e tests otherwise you will get wrong test results.

No problem for me. Let us better keep everything simple.

@4ydan 4ydan force-pushed the documentation/783-improve-e2e-docu branch from a0f4123 to 5b84d43 Compare August 4, 2023 18:32
@4ydan 4ydan requested review from Bushuo and absurd-turtle August 4, 2023 18:45
@4ydan
Copy link
Contributor Author

4ydan commented Aug 4, 2023

Let me know if anything is unclear/complicated or I have forgotten something and if you can run e2e tests locally. Thank you!

@4ydan 4ydan force-pushed the documentation/783-improve-e2e-docu branch from 5b84d43 to af94400 Compare August 4, 2023 18:54
Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

Great job, both make test-e2e without Docker and "e2e.sh" with Docker now works for me. 🚀 A few minor comments, can be also fixed in a later PR.

e2e/e2e.sh Outdated Show resolved Hide resolved
e2e/clean_db.py Outdated Show resolved Hide resolved
e2e/README.md Outdated Show resolved Hide resolved
e2e/.env Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@4ydan 4ydan force-pushed the documentation/783-improve-e2e-docu branch 3 times, most recently from 7b41420 to 3eacbc8 Compare August 5, 2023 11:58
@4ydan 4ydan force-pushed the documentation/783-improve-e2e-docu branch from 3eacbc8 to bd2862b Compare August 5, 2023 12:09
@4ydan 4ydan mentioned this pull request Aug 5, 2023
22 tasks
Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

LGTM, let us merge and do a follow-up when needed.

@@ -0,0 +1,3 @@
E2E_URL=localhost:5173
Copy link
Contributor

Choose a reason for hiding this comment

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

Some documentation & DATABASE_URL missing.

@@ -0,0 +1,4 @@
#!/bin/bash

Copy link
Contributor

Choose a reason for hiding this comment

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

Please feel free to readd the e2e/clean_db.py.

@markus2330 markus2330 merged commit e8aa9fc into master Aug 6, 2023
@markus2330 markus2330 deleted the documentation/783-improve-e2e-docu branch August 6, 2023 04:41
@4ydan 4ydan added the e2e regarding end to end tests label Aug 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
e2e regarding end to end tests please merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve e2e docu cannot run e2e tests locally
2 participants