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

Docs: Improve documentation with regards to running tests locally #613

Conversation

drikusroor
Copy link
Contributor

@drikusroor drikusroor commented Nov 13, 2023

This PR aims to add the following:

  • Add a github actions test workflow status badge (see image on of README.md).
  • Document how to run the frontend and backend tests locally.
  • Separate the frontend and backend tests in the github actions workflow so that it will be easier to see which one fails (see image below or here).
  • Run test workflow on both push and pull_request events.

282596281-61f3c53c-f37f-46f4-93a9-8fc59806dc13

@drikusroor
Copy link
Contributor Author

Btw, I'm thinking about the idea of not using docker run but docker exec to run the tests locally as docker run creates a new container instance. This is now clogging up my container list in the GUI (see screenshot). What would you think of that idea?

afbeelding

Copy link
Contributor

@albertas-jn albertas-jn left a comment

Choose a reason for hiding this comment

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

The idea to separate back-end and front-end tests is good, but:

  1. I think running tests both on push and pull_request is unnecessary. In our workflow, pushing is usually followed by pull request, so the same tests will be running twice.
  2. Our front-end tests do not need the back-end tests. They can be run independently.
  3. We already have test-back and test-front scripts set up, perhaps documentation should reflect that.
  4. @BeritJanssen can you comment on docker exec vs docker run?

@drikusroor drikusroor self-assigned this Dec 3, 2023
@drikusroor drikusroor added documentation Improvements or additions to documentation Infrastructure Relates to deployment or testing labels Dec 3, 2023
@drikusroor
Copy link
Contributor Author

drikusroor commented Dec 3, 2023

The idea to separate back-end and front-end tests is good, but:

1. I think running tests both on push and pull_request is unnecessary. In our workflow, pushing is usually followed by pull request, so the same tests will be running twice.

2. Our front-end tests do not need the back-end tests. They can be run independently.

Good points! I've updated the workflow accordingly. With regards to point 1.: I've edited it so that the workflow will run for a PR and for every push to develop and main (which should normally happen after a merge). Would you be okay with that or did you have something else in mind?

3. We already have `test-back` and `test-front` scripts set up, perhaps documentation should reflect that.

Fair point as well. I was looking at the commands in those scripts and realized Windows doesn't have sudo. Would you happen to have a good solution to that, or is the sudo prefix even necessary for executing the tests inside of the Docker environment?

@BeritJanssen
Copy link
Collaborator

As the name of the container is not the same across all users, I prefer docker-compose run over docker exec (otherwise the instruction would become: "run docker ps, find name of container, then run docker exec {container_name}, a bit more wordy and nothing users can copy-paste). I now attach the --rm flag across all documentation in the wiki, which makes sure the containers will be removed after the tests finish.

As with #620, I'd also request that documentation on how to run the tests be moved to the wiki, so as not to clutter the README.

@drikusroor
Copy link
Contributor Author

As the name of the container is not the same across all users, I prefer docker-compose run over docker exec (otherwise the instruction would become: "run docker ps, find name of container, then run docker exec {container_name}, a bit more wordy and nothing users can copy-paste). I now attach the --rm flag across all documentation in the wiki, which makes sure the containers will be removed after the tests finish.

As with #620, I'd also request that documentation on how to run the tests be moved to the wiki, so as not to clutter the README.

Ah yes, with the --rm it wouldn't clog up my list of containers. By the way, you mention here (and in #620) that "the name of the container is not the same across all users". Has this been an issue you are running into or was this a conscious decision? I believe (from the top of my head) that the container name is configurable in the docker-compose.yml file.

@drikusroor drikusroor force-pushed the docs/update-readme-test-command branch from 0827aa9 to dc56187 Compare December 5, 2023 13:52
@drikusroor drikusroor merged commit 42b67e1 into Amsterdam-Music-Lab:develop Dec 5, 2023
2 checks passed
@BeritJanssen
Copy link
Collaborator

Ah, I never realized you could set the container name. So no, no conscious decision. But what would happen if somebody spins up multiple containers? You would then still get server_1, server_2 and so on? I think that last bit is what Docker attaches nonetheless, and I noticed that it can differ whether the index is slapped on with hyphen or underscore. Perhaps there's a setting for that, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation Infrastructure Relates to deployment or testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants