Get tests running and ActiveRecord::Wrapper working again plan

jrochkind commented 5 years ago

Currently, tests do not run bundle exec rake test has errors that keep tests from even running. I believe travis is not running.

Additionally, for the ActiveRecord::Wrapper class, tests are only run in Rails 4.0.x. (Note that's not 4.x, but 4.0.x).

Once I hack the test suite to run -- I can only get ActiveRecord::Wrapper-related tests to pass in Rails 4.1.x, slightly different than the 4.0.x they run in but still ancient.

I believe non-AR-related functionality is still working (with passing tests). But AR-related functionality is not working on Rails 4.2+.

My plan to get things back into shape:

[x] Get AR func working and tested on AR 5.2.x and 6.0.x
- These are the only currently supported by Rails AR versions, with the release of 6.0.0, even for severe security issues. Note that the AR func was only working in Rails 4.1.x at present, which is long out of support.
- In order to do this, first get tests passing on AR 4.1.x. Then get the passing on 4.2, 5.0... 5.2. Then add appraisal to test on both 5.2 and 6.0.
[x] Turn travis testing on, with icon on github README
[x] Look through open issues/PR's to see if there's anything still relevant and feasible to address
[x] Do a 1.0 release, this is a dependency of dozens of apps in production, it should be at 1.0 with the semver backwards compat commitment.

Note that there is no plan to touch non-AR functionality. Which is probably all any existing dependers are using, since AR func is pretty broken on Rails 4.2+. So this work should not disturb them.

NOT included in current plan

The tests are written in a very archaic Test::Unit style. They would be a lot easier to work with in rspec, but do not plan to do that as part of this push.
One could argue it would make sense to split AR-specific behavior out into a separate gem. After all, it is the main (only?) thing that caused tests to break, and the thing that will have to be kept up to date with Rails releases, while the core code remains more stable. And the majority/all current users of this gem are via blacklight-oai, not using AR functionality. This is a reasonable argument, but if we don't have the capacity to properly maintain this one gem, I don't think splitting into two -- that need to be kept in sync, when this one doesn't have a 1.0 release yet -- is the way to go. For now. For this push.

Collaboration?

I would be quite happy if anyone wanted to collaborate on this by giving feedback on directions, or reviewing PR's, etc. Please, you are invited! Let me know.

But before/without hearing from those, I'm going to forge forward with this plan. I have time to work on it this week, and need it for the app I am working on, and would like to give back to this project rather than fork.

I am aware of no policies or rules applying to code reviews or anything else for this project in code4lib org.

I do plan to do all my changes as PRs (NOT commit directly to master). I may merge my own PR's if there is nobody else with interest in reviewing them.

bess commented 5 years ago

I am totally in favor of this plan. Thank you for taking this on, @jrochkind! :rocket:

jrochkind commented 5 years ago

Correction -- it looks like AR-related tests will currently pass in AR 4.2.x only. (While they only RUN in 4.0.x, due to Gemfile).

So it's possible some people are using this in 4.2.0 with AR wrapper, and will not be able to do so with the new release anymore after this plan completes. They can of course keep using whatever version they are using, but planned 1.0.0 release will only support supported Rails versions: 5.2 and 6.0

jrochkind commented 5 years ago

There's SimpleCov stuff in there that I think nobody has used in years, and is hard for me to keep/ensure it working.

I am going to remove it. I don't think it is producing test coverage information in a way that is useful to anyone in 2019 anyway -- when I can get it mostly working, it just produces a whole bunch of HTML files (one for every class, including test classes that you don't actually want coverage on!) for you to review manually.

Rather than getting it doing something useful, I am just going to remove it for now. Test coverage can be added back later by someone else if desired, using modern tools and output formats.

jrochkind commented 5 years ago

master is basically ready to be a 1.0.0.beta release. I may release that soon, or not, let me know if you have opinions.

I will not release a 1.0.0 until I have it working satisfactorily for my needs in my own app, which will use the ActiveRecordWrapper.

jrochkind commented 5 years ago

If anyone wants to discuss #38 with me, please let me know.

The gem is constructing identifiers as [namespace]/[prefix]. But is construting a sample record in Identify as [namespace]:[prefix]. (colon instead of slash).

OAI guidance suggests the colon, but it is not a requirement just a suggestion.

But they should both match in this gem. I am inclined to switch it to colon to be in conformance with Guidance, even though that's technically a backwards-breaking change.

I think the change may not affect users of blacklight_oai_provider, as it overrides the relevant methods anyway (to use colon). In fact, it overrides them in a monkey-patch file whose commit says "Patch identifier bug in OAI gem", so apparently they considered it a bug -- yep, we should just fix it here.

jrochkind commented 5 years ago

If there's anyone that wants to PR review https://github.com/code4lib/ruby-oai/pull/76 , let me know! New feature, customize identifier_field, analogous in implementation to existing ability to customize timestamp_field.

jrochkind commented 5 years ago

A 1.0.0.beta1 has been released!

If I hear nothing else, I will release it no earlier than one week from today (Tuesday September 3rd), or no later than... when I remember to.

But I am happy to wait longer than that for a 1.0.0 release if there are people who want to test it or look at it and want more time, no problem at all, just let me know! I'd love to have people testing and looking at it!

I am also happy to get feedback on already merged PR's, especially prior to a 1.0.0 release. Just because it's already merged to master doesn't mean it's too late for feedback, I would love it!

So happy to slow things down and wait to give people time to do those things if people are going to do them -- but if I hear nothing in a week, I'll go ahead and release!

Closing this ticket, please follow or comment on #77 tracking the upcoming 1.0 release instead!

code4lib / ruby-oai