Replacing derby with lamprey (the integration test suite harness from rdf-ldp) #136
Conversation
|
I am opening this as a draft PR given that the test coverage may well not be sufficient. |
jrgriffiniii
force-pushed
the
issues-131-jrgriffiniii-ruby3.0
branch
2 times, most recently
from
450708e
to
12a4405
Compare
|
Please disregard this, inspecting https://app.circleci.com/pipelines/github/samvera/ldp/74/workflows/2e4b1ded-40ce-4117-bfbe-454fd0d93c4e/jobs/670/steps, I have found the following to be the current code coverage analysis results: Coverage report generated for RSpec to /home/circleci/project/coverage. 492 / 571 LOC (86.16%) covered. |
jrgriffiniii
force-pushed
the
issues-131-jrgriffiniii-ruby3.0
branch
from
c843617
to
cfe5fb4
Compare
|
Support for Ruby 2.5 was droppped for |
jrgriffiniii
force-pushed
the
issues-131-jrgriffiniii-ruby3.0
branch
from
1add5e4
to
4466368
Compare
jrgriffiniii
force-pushed
the
issues-131-jrgriffiniii-ruby3.0
branch
from
4466368
to
8c81e28
Compare
jrgriffiniii
force-pushed
the
issues-131-jrgriffiniii-ruby3.0
branch
from
2c5d4fe
to
c881e59
Compare
|
Thank you very much for the review @bess! Following a discussion regarding the substantive changes, I can proceed with adding this with a request for review to the Samvera Tech Call scheduled for this week: https://samvera.atlassian.net/wiki/spaces/samvera/pages/1954250753/Samvera+Tech+Call+2022-04-20 |
There was a problem hiding this comment.
This all looks really well done! Thanks for your effort on this!
I see that you've preserved the old functionality (utilizing derby?) by testing if the client has a repository set. Does it make sense to keep this support or can it be removed entirely? I guess asking differently, it looks like these changes are meant to be backwards-compatible, but should this just be a major release and drop the prior way of doing things? If not, do you think there is sufficient test coverage of both code paths? (I see there is testing of the new paths in the integration test and the old paths in the unit tests.)
I think it would also be good to run ActiveFedora's and Valkyrie's test suites against this branch as a smoke test.
spec/lib/ldp/client_spec.rb
Outdated
| end | ||
|
|
||
| it 'raises an ArgumentError with bad arguments' do | ||
| expect { Ldp::Client.new(nil, nil, nil) }.to raise_error ArgumentError | ||
| context 'with addition client options' do |
There was a problem hiding this comment.
It looks like this context label was copied from the previous and should be renamed. Here's a suggestion.
| context 'with addition client options' do | |
| context 'with invalid arguments' do |
There was a problem hiding this comment.
Thank you very much for this thorough review and guidance! I did indeed intend to preserve what I could of the method signatures and invocations in order to preserve backwards-compatibility for those adopters who cannot completely remove the derby dependency.
As such, I can now look to increase the test coverage for these proposed changes.
jrgriffiniii
changed the title
|
Superseded by #149 |
Resolves #131