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 docker setup for more consistent testing #77

Merged
merged 11 commits into from
Oct 31, 2023

Conversation

osingaatje
Copy link
Contributor

No description provided.

Copy link
Member

@pond pond left a comment

Choose a reason for hiding this comment

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

Really not sure about this one. "Consistent" testing almost certainly misses the point; Scimitar might be used on a wide range of Ruby versions and database combinations, even including non-ActiveRecord setups. The more varied the test environments people run, the greater the chance of finding bugs.

Of course, the project itself uses GitHub Actions to run tests so this won't affect them and is for sure only opt-in for end users, but there's nothing added in this PR to any of the docs to tell them why a gem has Docker setup present nor what they're supposed to do with it.

Notwithstanding these other issues, you'll certainly need to fix up docker-compose.yml since it appears to be Heroku-specific.

@osingaatje
Copy link
Contributor Author

Thanks for the feedback!

I agree with your point on consistent testing, it is quite useful that it is ran on a host of different ruby versions and setups, and databases.
I added this docker setup as a way for me to be able to run this gem at all, because I personally had quite a bit of trouble running the project (due to my lack of knowledge running PostgreSQL). I figured that there could be other people that have trouble running the project as well, so having a preconfigured image might be a nice alternative for people who have a hard time getting the project to run.

If this is something that would benefit the Scimitar project, I can update the docs to reflect this (optional) way of running the project, mentioning that running it on one's own setup is preferred as this increases the chance of finding bugs.

Lastly, I'd like to thank you for making and maintaining this project, it's been proven to be quite useful for my project!

..to be able to do `binding.pry`, see https://github.com/pry/pry
@pond
Copy link
Member

pond commented Oct 30, 2023

I can update the docs to reflect this (optional) way of running the project, mentioning that running it on one's own setup is preferred as this increases the chance of finding bugs.

@osingaatje definitely, yeah, please do update README.md. I note a test script added into the dummy test app too, so you should be looking at the Tests section in line 622 (in current main) and doing a bit of work there to figure that out. It's possible that the dummy test script is a bit over-complex and could just be using pushd/popd itself to avoid that slightly awkward walk-back-up-tree thing. As it happens, there's a PR in progress that adds a bin folder at the top-level of Scimitar to help with testing - adds bin/console in #79. Perhaps move your new test script into a same top-level bin folder so that testing becomes a matter of just bin/test, rather than needing to use spec/dummy/apps/bin/test? That'll be "compatible" with PR 79 or an equivalent if that one eventually gets merged one way or another. Semantically I think it reads better too since it is a full gem test that's run, not just a test of the dummy app.

(As a side-note, I don't mind the introduction of Pry in there but we use byebug in Scimitar for debugging elsewhere which is already in the gem's development dependencies, dunno if that's of use/interest instead).

@osingaatje
Copy link
Contributor Author

osingaatje commented Oct 31, 2023

Thanks for the feedback. I'll update the 'Tests' part to highlight the choice between running the tests locally and using the Docker setup.

For the dummy test script, it is indeed a bit awkward to have to navigate back up to the root, but that is due to the ruby:3.2.2 container not recognising the commands pushd and popd.
I'll move the test script to bin/test, but I'm afraid I can't really use pushd and popd unless I change containers, or I'm missing some other step, I don't know, I'm not a Linux expert.

As for the Pry situation, apparently pr #79 also includes an optional pry include? I'm fine with deleting the dependency, since byebug seems to do roughly the same as pry, but it might also be worth commenting it out, or keeping it in entirely to allow for easier debugging for people not familiar with byebug. I'll let you be the judge of that : )

I'll quickly fix up the README and the test script.
(And I also noticed a warning in my Docker file suggesting I do apt-get clean after doing apt-get install, so I'll add that quickly as well, saves some resources in the Docker container)

the `docker compose build` gave a warning about the wrong bundle version being used to bundle the project, so changed that to make the setup a little smoother
@pond
Copy link
Member

pond commented Oct 31, 2023

I don't see anything about Pry in #79 but it doesn't matter either way, it's harmless enough - only part of the test application, commonly used and its dependency chain revealed by the Gemfile.lock updates is tiny.

So, this is good to go now! Merging. Many thanks for the contribution 😄

@pond pond merged commit b397200 into RIPAGlobal:main Oct 31, 2023
4 checks passed
@pond
Copy link
Member

pond commented Nov 15, 2023

This is now available via RubyGems in v2.6.0 / v1.7.0.

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.

None yet

2 participants