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 check for event on first frame #251

Merged
merged 7 commits into from
Jun 25, 2024

Conversation

psomhorst
Copy link
Contributor

Closes #81

I tested this by removing all data before a frame with an event from a BIN file.

I tested this by removing all data before a frame with an event from a BIN file.
@DaniBodor
Copy link
Member

Ideally we would add a test for this as well, to avoid someone in the future thinking: "why is this written so complicated" and then trying to clean it up therein breaking it again.
This would require adding another dataset to the environment though...

@psomhorst
Copy link
Contributor Author

Ideally we would add a test for this as well, to avoid someone in the future thinking: "why is this written so complicated" and then trying to clean it up therein breaking it again. This would require adding another dataset to the environment though...

This makes sense. If I supply it to you through another channel, could you add it to the test build? I'll write a test for it before merging.

@DaniBodor
Copy link
Member

DaniBodor commented Jun 25, 2024

Ideally we would add a test for this as well, to avoid someone in the future thinking: "why is this written so complicated" and then trying to clean it up therein breaking it again. This would require adding another dataset to the environment though...

This makes sense. If I supply it to you through another channel, could you add it to the test build? I'll write a test for it before merging.

@wbaccinelli , any chance you could do this, I don't remember by heart how to do it and would need to look up my notes somewhere.
Ideally, could you also write a quick how-to in the README.dev.md for this?

@psomhorst
Copy link
Contributor Author

psomhorst commented Jun 25, 2024

Ideally we would add a test for this as well, to avoid someone in the future thinking: "why is this written so complicated" and then trying to clean it up therein breaking it again. This would require adding another dataset to the environment though...

This makes sense. If I supply it to you through another channel, could you add it to the test build? I'll write a test for it before merging.

@wbaccinelli , any chance you could do this, I don't remember by heart how to do it and would need to look up my notes somewhere. Ideally, could you also write a quick how-to in the README.dev.md for this?

I was able to add the data to the package myself.

  • Make a personal access token (https://github.com/settings/tokens) with "read:package" and "write:package" rights
  • > docker login ghcr.io -u <username> - use token as password
  • > docker pull ghcr.io/eit-alive/eittestdata:latest
  • > docker run -i -t --name eittestdata_container ghcr.io/eit-alive/eittestdata:latest /bin/bash
  • In a separate terminal:
    • > docker cp <file path> eittestdata_container:/eitprocessing/tests/test_data
    • > docker commit eittestdata_container ghcr.io/eit-alive/eittestdata:<TAG>
  • In running docker container: exit
  • > docker push ghcr.io/eit-alive/eittestdata:<TAG>

@DaniBodor Is there a way to test whether the tests work with the tag add-data before pushing it as latest?

I pushed it as latest (with the option to remove it and restore the previous image as latest). All tests kept running, including a test using the new file.

This only changes an existing image. It would be better if we would have the DockerFile that creates it from 0.

@psomhorst psomhorst merged commit 7c07575 into develop Jun 25, 2024
5 checks passed
@psomhorst psomhorst deleted the 081_fix_event_on_first_frame_psomhorst branch June 25, 2024 09:22
@wbaccinelli
Copy link
Contributor

wbaccinelli commented Jun 25, 2024

Ideally we would add a test for this as well, to avoid someone in the future thinking: "why is this written so complicated" and then trying to clean it up therein breaking it again. This would require adding another dataset to the environment though...

This makes sense. If I supply it to you through another channel, could you add it to the test build? I'll write a test for it before merging.

@wbaccinelli , any chance you could do this, I don't remember by heart how to do it and would need to look up my notes somewhere. Ideally, could you also write a quick how-to in the README.dev.md for this?

I can add a new dataset to the Docker image, just tell me which one. It's a good idea to add the process to the documentation. We probably also need to tag the images in a smart way, to avoid corrupting the test by mistakenly uploading a new faulty image.

@wbaccinelli
Copy link
Contributor

Ideally we would add a test for this as well, to avoid someone in the future thinking: "why is this written so complicated" and then trying to clean it up therein breaking it again. This would require adding another dataset to the environment though...

This makes sense. If I supply it to you through another channel, could you add it to the test build? I'll write a test for it before merging.

@wbaccinelli , any chance you could do this, I don't remember by heart how to do it and would need to look up my notes somewhere. Ideally, could you also write a quick how-to in the README.dev.md for this?

I was able to add the data to the package myself.

  • Make a personal access token (https://github.com/settings/tokens) with "read:package" and "write:package" rights

  • > docker login ghcr.io -u <username> - use token as password

  • > docker pull ghcr.io/eit-alive/eittestdata:latest

  • > docker run -i -t --name eittestdata_container ghcr.io/eit-alive/eittestdata:latest /bin/bash

  • In a separate terminal:

    • > docker cp <file path> eittestdata_container:/eitprocessing/tests/test_data
    • > docker commit eittestdata_container ghcr.io/eit-alive/eittestdata:<TAG>
  • In running docker container: exit

  • > docker push ghcr.io/eit-alive/eittestdata:<TAG>

@DaniBodor Is there a way to test whether the tests work with the tag add-data before pushing it as latest?

I pushed it as latest (with the option to remove it and restore the previous image as latest). All tests kept running, including a test using the new file.

This only changes an existing image. It would be better if we would have the DockerFile that creates it from 0.

I just read this comment. In genera, it might be easier to just recreate the Docker image using the Dockerfile and then push it with the tag we want.

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.

3 participants