-
Notifications
You must be signed in to change notification settings - Fork 5
Fixes Issue #8 #10
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
base: main
Are you sure you want to change the base?
Fixes Issue #8 #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etagwerker, Great addition, but it broke CI, could you fix it please?
| gem_version = gem_version.include?(",") ? gem_version.split(", ").map { |i| "'#{i}'" }.join(", ") : gem_version | ||
|
|
||
| result += "gem '#{gem_name}', #{gem_version}\n" unless gem_name == "rails" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etagwerker, I opened a PR with the following fix, and now CI is passing
#11
| gem_version = gem_version.include?(",") ? gem_version.split(", ").map { |i| "'#{i}'" }.join(", ") : gem_version | |
| result += "gem '#{gem_name}', #{gem_version}\n" unless gem_name == "rails" | |
| if gem_version.include?(",") | |
| gem_versions = gem_version.split(", ").map { |i| "'#{i}'" }.join(", ") | |
| result += "gem '#{gem_name}', #{gem_versions}\n" unless gem_name == "rails" | |
| else | |
| result += "gem '#{gem_name}', '#{gem_version}'\n" unless gem_name == "rails" | |
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etagwerker, For some reason I cannot run it locally, but my guess is that we are missing the single quotes in the result when there is a single version.
- Added instructions for running tests locally - Update Ruby so we can use a more recent version of bundler in CI Fixes: #8
Introduce `spec:vcr_record_new` rake task to refresh all VCR cassette files used in tests. Update README with clear instructions for re-recording HTTP interactions. Improve VCR configuration to support cassette re-recording and ensure bundler cache is cleared before tests when needed.
|
Hello @etagwerker I added two commits to this PR. I am not sure if adding VCR is worth it. I added a fair bit of instructions for recording cassettes locally - however if you are on a different OS than the one used in CI then that too could cause tests to fail on CI when different gems are pulled in CI (x86) vs locally (m-chip). To get the tests to pass I generated the cassettes in CI, downloaded them and commited them to this PR. What do you think, do we keep the cassettes for now, or leave it out of this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fbuys thanks for taking a stab at this!
Unfortunately, I don't think it's a good idea to add vcr/webmock if it means that the test suite only passes in CI and not locally. It will cause frustration for anyone contributing to this project and it will be an unpleasant surprise for anyone trying to add a small change.
I'm getting these errors locally: https://gist.github.com/etagwerker/fe3f628fa8f7213c922d2f9bbe2cad49
I do like the idea of adding vcr/webmock if you can figure out a way for the test suite to pass both in CI and a local dev environment.
@fbuys this sounds like a bug in VCR, did you try reporting it to that project? I haven't found anything related to this in their project. |
Hey @JuanVqz,
This PR fixes #8 based on the pair session we did with Ryan.
Please take a look.
Thanks!