Extend doorkeeper; run its specs in this extension gem #2
Conversation
| s.summary = "Doorkeeper with extracted ORM specifics, including mongoid 2-4 and mongo_mapper." | ||
| s.description = "Doorkeeper with extracted ORM specifics, including mongoid 2-4 and mongo_mapper" | ||
| s.summary = "Doorkeeper mongoid 2, 3, 4 and mongo_mapper ORMs" | ||
| s.description = "Doorkeeper mongoid 2, 3, 4 and mongo_mapper ORMs" |
There was a problem hiding this comment.
We should probably extract these in two different repos.
There was a problem hiding this comment.
Is the plan to have one repo for each ORM? If so, how will we make sure each repo can be tested against the same Doorkeeper tests/specs?
There was a problem hiding this comment.
This is the critical problem.
You may found specs about models, by theory you implement a ORM support, and pass specs for your job, that's enough to proving the correctness. but for Doorkeeper it's can not.
As my opinion, I refactor models and extract ORM support, but that's not enough, we don't have a obvious interfaces for models to tell how to play with them, so developers can not know what they need to implement and what the behaviour are. this point also discussion in PR #281.
so I think the critical thing is define these interfaces and those behaviours, and we could make a suppose that if a developer implement those correctly, the doorkeeper will work correctly.
There was a problem hiding this comment.
I address those concerns in the load_doorkeeper task. It's far from elegant I think, and it has the problem of ORM specifics in doorkeeper's tests, but there is the single canonical test suite, that should run on each ORM extension repository through a clone. ORM extensions wouldn't have much -if any- specs of their own.
The clone should be smarter (get from GitHub a certain tag, clone only the specs). Now spiking to see if git submodules would be a good approach.
There was a problem hiding this comment.
How about just specifying the Doorkeeper repo with an ENV variable? As an ORM gem maintainer, I would have my own Doorkeeper repo which I could then check out any branch or SHA, then cd into the ORM gem's repo and run something like doorkeeper_src=../doorkeeper bundle exec rspec. As long as this is documented, I think the flexibility would outweigh the inconvenience.
If we do a submodule, the submodule would have to be updated any time you want to test against a different version of doorkeeper, right?
There was a problem hiding this comment.
How about just specifying the Doorkeeper repo with an ENV variable? As an ORM gem maintainer, I would have my own Doorkeeper repo which I could then check out any branch or SHA, then cd into the ORM gem's repo and run something like doorkeeper_src=../doorkeeper bundle exec rspec. As long as this is documented, I think the flexibility would outweigh the inconvenience.
This is not far enough from what we have now, but if it's not automated it just won't be reliable enough, I don't think.
If we do a submodule, the submodule would have to be updated any time you want to test against a different version of doorkeeper, right?
I just pushed the submodule idea. I don't see this as a problem, as I do expect the ORM extensions and doorkeeper to go hand in hand with versioning, and to have quite strict versioning restrictions. Hopefully it's not needed, but I wouldn't be surprised if there's a minor release of minor versions almost per minor release of doorkeeper.
It doesn't need to be tested against each commit, but against the relevant tag (v2.2.0 as of now).
There was a problem hiding this comment.
We should probably extract these in two different repos.
I won't extract them into different repos; the project is not more complex because it contains both mongomapper and mongoid, and they share dependencies setup.
There was a problem hiding this comment.
@tute Would you like Sequel in this repo as well, or in a separate gem? A separate gem creates one more system to maintain, but I wasn't sure how to make the "dummy" app in the current Doorkeeper ORM setup work with both Sequel and ActiveRecord migrations. So, forgetting about how Doorkeeper currently is, how would you like it to be?
tute
force-pushed
the
extract-orms
branch
3 times, most recently
from
f72051b
to
049e71c
Compare
This PR removes doorkeeper ORM specifics, in favor of extending it with other rubygems. The specs defined in this project are still considered canonical, and the goal is for extension rubygems to set doorkeeper as a git submodule, and running its test suite against the specific ORM that is being extended. See doorkeeper-gem/doorkeeper-mongodb#2
tute
changed the title
Project (and files) rename to doorkeeper-mongodb hid the diff discussion, and answered your question. I want to work with you on a sequel separate extension. Your question is the next one I have to address in doorkeeper-gem/doorkeeper#648 (comment): what do we do with the unavoidable ORM specifics that the dummy app require? Loud thinking, I believe https://github.com/doorkeeper-gem/doorkeeper/blob/master/spec/dummy/app/models/user.rb might be the only spec file ORM extensions need to define, but I didn't implement that yet. If we manage to do something like that I think it would be perfect, and it would require some documentation to explain to extension developers how the tests fit together. Does it make sense to you? |
|
It's starting to make more sense. So the dummy Rails code would stay in the main Doorkeeper code base with the rest of the ORM specs, and anything ORM-specific to it would somehow be defined in the ORM-specific gem, yes? Bear in mind that migration code is ORM-specific also. |
tute
force-pushed
the
extract-orms
branch
2 times, most recently
from
a0f15bb
to
f4687f7
Compare
That is right. Doorkeeper will be ActiveRecord by default, with ActiveRecord migrations, with Rails and Ruby specs (as little ORM specifics as possible, if any). Extension gems will provide ORM specifics for doorkeeper models, ORM specific generators (still need to move them), and as little extensions to the doorkeeper tests as possible (mainly, the Thanks for your questions and feedback! |
|
Cool. Thanks for working on getting all this in place. It might be best for me to wait on starting the Sequel extension until the Mongoid and Mongo-Mapper extension is all done and working how you picture it should. I will definitely need help getting started, but from there it should just be a matter of making specs go from red to green. |
|
Good, this may the fastest way to extract ORM codes to a standalone gem, I was researching the way but forgot git-submodule. But I have an idea that define interfaces to tell how other part of Doorkeeper should to interactive with models, so if these interfaces have a stable and clear behaviour, for ORM support developer they just implement them and test them, no need to run the whole specs of Doorkeeper its self. |
tute
force-pushed
the
extract-orms
branch
4 times, most recently
from
e3aa2d0
to
8260d15
Compare
This PR removes doorkeeper ORM specifics, in favor of extending it with other rubygems. The specs defined in this project are still considered canonical, and the goal is for extension rubygems to set doorkeeper as a git submodule, and running its test suite against the specific ORM that is being extended. See doorkeeper-gem/doorkeeper-mongodb#2
This PR removes doorkeeper ORM specifics, in favor of extending it with other rubygems. The specs defined in this project are still considered canonical, and the goal is for extension rubygems to set doorkeeper as a git submodule, and running its test suite against the specific ORM that is being extended. See doorkeeper-gem/doorkeeper-mongodb#2
doorkeeper 3 has not been released yet, and will be when its ORMs are successfully extracted and tested into this project. This PR extends doorkeeper with the ORM specifics for mongoid and mongomapper. To test it, it loads doorkeeper as a submodule, copy its test suite into this project, configures the ORMs according to the matrix defined in `.travis.yml` file, and run the test suite with each. Renames project from `doorkeeper-orm` to `doorkeeper-mongodb`.
tute
force-pushed
the
extract-orms
branch
4 times, most recently
from
2d92c80
to
46b8f52
Compare
This PR removes doorkeeper ORM specifics, in favor of extending it with other rubygems. The specs defined in this project are still considered canonical, and the goal is for extension rubygems to set doorkeeper as a git submodule, and running its test suite against the specific ORM that is being extended. See doorkeeper-gem/doorkeeper-mongodb#2
Extend doorkeeper; run its specs in this extension gem
Related PR: doorkeeper-gem/doorkeeper#648
After months of inactivity in this project, this PR:
rakeis run, it git-submodules doorkeeper into this project, copies it specs into this project, configures each ORM as it did before, and runs the matrixIf successful, this changes will guarantee good test coverage, while offloading the main project from ORM specifics, and setting the framework for extending doorkeeper with other ORMs in the future, like Sequel.
Both projects see a good portion of their code bases removed, keeping same behavior, test coverage, and getting faster specs. WIN.