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

[BUG] Fixing dockerized tests #5385

Closed

Conversation

kurayami07734
Copy link
Contributor

Reference Issues/PRs

Fixes #5352

What does this implement/fix? Explain your changes.

Tests fail to run because of a git diff command in the tests and the docker image is not a git repo

What should a reviewer concentrate their feedback on?

Currently I've added a new build target in the Makefile full_test which runs all the tests.
What else should I do?

@kurayami07734 kurayami07734 marked this pull request as ready for review October 8, 2023 18:08
Copy link
Collaborator

@yarnabrina yarnabrina left a comment

Choose a reason for hiding this comment

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

I'm sorry, but I don't see how any of these changes affect the mentioned issue. It looks same as your previous PR #5356, which is already merged.

Please let me know if I'm missing something.

@fkiraly
Copy link
Collaborator

fkiraly commented Oct 10, 2023

Might be work in progress still? In that case, the protocol would be to make the PR a draft to signal that you are still working on it.

When you open (green button, dropdown), or at the top right, here:
image

@kurayami07734 kurayami07734 marked this pull request as draft October 10, 2023 09:46
@kurayami07734
Copy link
Contributor Author

I'm sorry, but I don't see how any of these changes affect the mentioned issue. It looks same as your previous PR #5356 , which is already merged.

I had just removed the dockerfile for python 3.7 in that PR

I wanted feedback on how I should configure the tests in the dockerfiles
@yarnabrina Had suggested adding a full_test target in the makefile

which runs all the tests, I also wanted to have smaller tests which run a smaller subset of the tests

Once the tests have been fixed, I will also make a PR for updating the docker images to bookworm

@yarnabrina
Copy link
Collaborator

image

This is what I see in the "Files Changed" section of this pull request. @kurayami07734 can you please confirm if this is up to your expectations?

Your comment above seems to suggest that you have made changes in Makefile and etc., similar to what we discussed in Discord. Is there any chance that you wanted to create the PR from some other branch or some changes are not pushed yet?

@kurayami07734
Copy link
Contributor Author

My changes seems to have gotten lost
Let me close this pr and open a fresh pr

@kurayami07734 kurayami07734 deleted the fixing-dockerized-tests branch October 13, 2023 19:17
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.

[BUG] Dockerized tests not running
3 participants