-
Notifications
You must be signed in to change notification settings - Fork 50
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably extract these in two different repos.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think? @tute
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
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
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. |
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. |
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`.
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:
rake
is 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.