Skip to content
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

Version 2.0 Multiple gems direction #24

Open
grantspeelman opened this issue Jun 25, 2019 · 18 comments
Open

Version 2.0 Multiple gems direction #24

grantspeelman opened this issue Jun 25, 2019 · 18 comments
Assignees

Comments

@grantspeelman
Copy link
Collaborator

So for version 2.0 I was thinking that we could take it a little further than just deprecating CypressDev. I think we should actually split this gem up into multiple gems. Looks like everyone wants tighter integration as where when I updated gem I made it more of a starter pack. ie generates a whole lot of files for you and you can modify them as you see fit.

Therefore I see the following new gems being born (names maybe need work 😄 ):

cypress-factory_girl (Cypress::FactoryGirl)
This being the main thing at the moment that everyone seems interested in.
Basically a rack application that turns your factory girl factories into a simple API with a easy way to interact with it from cypress

cypress-active_record_fixtures (Cypress::ActiveRecordFixtures)
make it easy to load your fixtures from cypress

cypress-scenarios (Cypress::Scenarios)
simple way to execute ruby files and "scenarios" from cypress

cypress-class_execute (Cypress::ClassExecute)
make it easy to interact with your model classes (and other classes) directly from cypress
this is basically what i currently do to generate data, I create wrapper classes around the normal models to make it easy to create data and takes advantage of rails class loading and unloading.

*cypress-on_rails (Cypress::OnRails)
this gem basically pulls all the other gems together and provides generators and binaries to get easily setup with rails

I like this approach because it allows to easily create new extensions to whats available and makes it possible to still use the different pieces with other ruby frameworks.

Or course there are pros and cons to this approach, so open to everyones thoughts and if this makes sense to go this direction.

@grantspeelman
Copy link
Collaborator Author

@yagudaev and @justin808 let me know what you think.

@justin808
Copy link
Member

  • This approach would be like how the rspec gem has several parts
  • The main disadvantage to consider would be increased complexity from having different versions of the gems interacting.
  • There's no runtime advantage to less code loading since this is only for testing
  • The main advantage is that we can have separate issues, PR's, releases for the different parts, which may make it a bit easier to fork and contribute to the gem.

Rspec-expectations has this in the gemspec:

   s.add_runtime_dependency "rspec-support", "~> #{RSpec::Expectations::Version::STRING.split('.')[0..1].concat(['0']).join('.')}"

to pin the common code dependency...

  1. How important is it to test the different combinations of versions?
  2. We would need some document that would explain how all the versions interact.

If the codebase is small enough, which it currently is, would just having one master branch be good enough? I'll think more about this on the weekend.

@grantspeelman
Copy link
Collaborator Author

I thought about it a bit more, going as far as having separate gems might not be worth it yet.
We could maybe just have these boundaries internally in the current gem. That might be enough for us to test the idea and it's something we can move to in safe steps 👍

@MUTOgen
Copy link

MUTOgen commented Jul 2, 2019

@grantspeelman @justin808 I don't think that splitting gem into several gems would be reasonable right now. Based on my experience with Cypress in Rails, i think the most valuable thing right now is to organize workflow with backend data.

Basically, we have 3 possible sources of data:

  1. Direct FactoryBot calls
  2. Seeding with Cypress fixtures (json)
  3. Seeding with Rails fixtures (yml)

We could consider possible wrappers around that scenarios.

One particular thing i was thinking about is ability to partly reseting DB. For some cases it would speed up testing, b/c reseting and re-seeding sometimes look pretty heavy.

Also having a way to slightly update some data would be really nice feature. I use appEval in some of before hooks to execute Ruby code for now (for example, to setup some flags or options for model). This improvement might be helpful too.

@grantspeelman
Copy link
Collaborator Author

yes, I think splitting it right now might be too early but starting to have the separation inside the gem is where we could start.

@grantspeelman
Copy link
Collaborator Author

Can you elaborate more on the partly reseting of the DB? is this not something that database_cleaner already provides, ie so won't updating the clean.rb be enough?

Or are you thinking tighter integration with database_cleaner so you can specify it's options from within cypress.

For example

cy.appCleanerStrategy('truncation', { only:  ["widgets", "dogs", "some_other_table"] })
cy.appCleanerClean()

for the updating data part, maybe something like?

# updating
cy.appUpdate([
  ['Post', 23, { name: 'Cypress on Rails is cool' }],
  ['User', { find_by: { email: "[email protected]" } }, { name: 'Grant' }] 
])

# destroying
cy.appDestroy([
  ['Post', 23 ],
  ['User', { where: { active: false } }] 
])

# or both
cy.appActiveRecord([
  ['Post', 'update', 23, { name: 'Cypress on Rails is cool' }],
  ['Post', 'destroy', 23 ],
  ['User', 'update', { find_by: { email: "[email protected]" } }, { name: 'Grant' }],
  ['User', 'destroy_all', { where: { active: false } }] 
])

We could create 2 new issues for these and flesh out the examples more and then start looking at how we can implement them.

@MUTOgen
Copy link

MUTOgen commented Jul 3, 2019

@grantspeelman Yeah, your examples looks really good. 👍

is this not something that database_cleaner already provides, ie so won't updating the clean.rb be enough?

For sure, it could be done by adding some logic to .rb files. Dealing with Ruby code each time, to implement such boilerplate operations looks not too easy. So, it looks like good place to improve.

My idea was to provide some clean and simple integration with Cypress. In "perfect world" i could use only cy object to handle any kind of such operations.

@justin808
Copy link
Member

Moved the database cleaning discussion to #26

@justin808
Copy link
Member

After a long discussion with @grantspeelman in a zoom call. The major issue with v1 is that too many files are generated and thus any gem upgrades are difficult to apply.

The solution is to create a complementary node package that encapsulates much of what is generated so that the generator just provides the required configuration files, like cypress_helper.rb with some simple hello world style examples.

We also want to bring some of the files in the lib/generators/cypress_on_rails/templates/spec/cypress/app_commands into the gem.

CC: @MUTOgen

@yagudaev
Copy link
Contributor

I like the idea of the NPM module to encapsulate all the generated commands. Great discussion guys.

Sorry for being late to the party, been busy trying to launch our product at work.

@justin808
Copy link
Member

@yagudaev Please email me for a slack invite, and we can include you in our next zoom call.

@vfonic
Copy link
Contributor

vfonic commented Apr 4, 2020

Sorry for coming late to the party. :)

Thanks for the great gem! I really love the simplicity of app commands and scenarios. I just started using this gem and I feel at home already. Thank you! :)

I had a little bit bumpy road starting to use this gem. The initial onboarding experience for me was a bit "off". I had to go into generated files and remove/refactor the code.

I think having all these checks in ruby files for DatabaseCleaner and FactoryBot or FactoryGirl or SmartWrapper, make the code harder to read and harder to maintain.

Couple of examples:

  1. Assuming there's no DatabaseCleaner (this gem not depending on DatabaseCleaner) we have to write code like this:
    https://github.com/shakacode/cypress-on-rails/blob/master/lib/generators/cypress_on_rails/templates/spec/cypress/app_commands/clean.rb#L6-L7

This code assumes, if user doesn't use DatabaseCleaner, he has Post model in their codebase. It also seems to be an ActiveRecord-compatible API (.destroy_all). Not to mention that .destroy_all could have undesired side-effects, especially for someone who has their tests set up without DatabaseCleaner.

  1. Assuming there's no ActiveRecord, we have to write code like this:
    https://github.com/shakacode/cypress-on-rails/blob/master/lib/generators/cypress_on_rails/templates/spec/cypress/app_commands/activerecord_fixtures.rb#L16-L21

We should have that else-part removed or at least commented out.

  1. I'd also prefer to have this commented out: https://github.com/shakacode/cypress-on-rails/blob/master/lib/generators/cypress_on_rails/templates/spec/cypress/app_commands/scenarios/basic.rb

After running the gem's install generator, I don't want to have any code in my app that doesn't work or wouldn't work if I remove some other gem from my app (DatabaseCleaner or ActiveRecord or FactoryBot or any other). Actually, I don't want to ever have any code in my app generated by a gem that doesn't work.

  1. Do you know anyone who is actually using this gem together with FactoryGirl? If not, why don't we drop support for that gem? I didn't check, but I'd bet that there's the first version of FactoryBot that's 100% API-compatible with FactoryGirl and assuming someone might use this gem and FactoryGirl is a nice gesture, but unnecessary IMO. YAGNI. The gem is called cypress-on-rails, we could imply some convention-over-configuration. When we get "client request" for additional support of some less usual setup, we can expand the functionality of the gem.

  2. In here we prefer FactoryGirl over FactoryBot (when both constants are available): https://github.com/shakacode/cypress-on-rails/blob/master/lib/generators/cypress_on_rails/templates/spec/cypress/cypress_helper.rb#L22-L24

I'm not sure in what scenario we'd have both FactoryGirl and FactoryBot, but this is a great example of why we should, in my opinion, be more strict about what we support with this gem.

  1. Why does this gem create cypress examples? #48

  2. (Slightly unrelated to the other points)
    Do we need this? https://github.com/shakacode/cypress-on-rails/blob/master/lib/generators/cypress_on_rails/templates/spec/cypress/app_commands/clean.rb#L10

Does this actually always work?
We're depending on a hard-coded string in two different files. It would be great to at least have that string as a constant.
But, what if someone runs tests in random order? What if tests run in parallel? Could it happen that a model test gets in between two cypress tests and then the stored output would show logs from model test and the failed cypress test.
Maybe this could be simply added to README or this could actually be the first cypress-on-rails extracted gem.

We could maybe use around block and have logger redirect to some our custom IO stream, which will, after the test ends, flush the contents to (rails) logger or dump file, like it does now. Except that perhaps, IO stream approach would be less error-prone. Maybe we don't even need IO stream and can just use around block and output whatever got added during the yield in around block?

@grantspeelman
Copy link
Collaborator Author

@vfonic thanks for you input.
My initial thinking was that the generated files was more of examples of "what you can do" more than actually being part of cypress-on-rails. The running of ruby files that are within your cypress directory through cy.app is all I was thinking cypress-on-rails needed to be. People are using FactoryBot and Fixtures , even though I don't, so thus the generated examples.

So that's a bit about the background and my original thinking.
I do now understand that there is a call for cypress-on-rails to be more than that and integrate tighter with these various gems and give a smoother out of the box experience.

Hopefully this helps with context

@grantspeelman
Copy link
Collaborator Author

@vfonic on your point number 7
Yes it works and I am currently using it 😸

@vfonic
Copy link
Contributor

vfonic commented Apr 7, 2020

@grantspeelman Thanks for the background info! It definitely makes sense.

I see now that cypress generates those files when you first run yarn run cypress (w/o cypress-on-rails).
I think the least surprise here would be if at least this would be written somewhere in the README. I favor the option where we explain to end-user (dev) how to create those "what you can do with cypress" files, but don't create anything immediately. What if someone already uses cypress and just discovered cypress-on-rails gem?

I agree with you, tighter integration of this gem and smoother experience seems to be the way to go.
My suggestion is to leave out of the gem (or gem generator output) any non-essential stuff. The logging (lucky number 7 from above) seems quite different from what I've seen in other gems. That's why I think it could be a part of README/docs. Also any code that refers to DatabaseCleaner or FactoryBot should not reach the end-user (developers using the gem), if those gems are optional.
Imagine for a second I've never heard of DatabaseCleaner or FactoryBot. When I run rails g cypress_on_rails:install, I see these constants in generated files. Are these classes/modules of this gem? 🤷‍♂
Would it be possible to have these files inside of the gem?

This approach still keeps only one gem, while providing better/cleaner DX, in my opinion.

@grantspeelman
Copy link
Collaborator Author

Yes, this is the plan for version 2.

  • Pull the ruby generated files into the gem
  • Have a matching npm package to support a more rich experience

This will allow a more integrated ("Just Works") experience with the various "tools".
"tools" being thinks like ActiveRecord, DatabaseCleaner, FactoryBot, ActionMailer, etc

This all said, pulling so many projects together and keeping update with all their changes is no easy task but could be a beautiful Development experience 😄

@vfonic
Copy link
Contributor

vfonic commented Apr 8, 2020

Thank you for your work!

I thought about it a bit more, going as far as having separate gems might not be worth it yet.

Maybe we can still ship v2 having everything in one gem? There are really not that many files. It would be a big overhead.
I think having these files pulled into the gem would already do wonders. We could have some predefined commands that would be defined in the gem. I'd just advise that by then we make sure that these commands have some limits in terms of what they can do ("sandboxed/restricted eval", basically what you guys have been working on in another PR).

@justin808
Copy link
Member

@vfonic Let me know if you'd like to give us a hand.

Feel free to join our Slack and DM me with this link: Slack Invite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants