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

Configure streaming spec via rspec tags #27932

Closed

Conversation

mjankowski
Copy link
Contributor

Related to #27882 (recently merged changes for search specs) and #27732 (capybara config) and conversation here - #27732

There changes here are similar to the search one...

  • Switch from using an ENV-var contingent run to an rspec tag run
  • Continue to opt-out of the default run for the system specs that need the streaming server
  • Configure CI to run them using the tag
  • It remains possible to run them locally by running with the tags

The branch this came out of also had the capybara config changes ... I've attempted to rebase and pluck out just the streaming system spec changes w/out the capybara changes; but if one of them is merged the other one will probably need a rebase to also go in. My preference would be merge capybara config first, but either way is fine.

@mjankowski
Copy link
Contributor Author

Future plans this enables:

  • Vague direction of following current rails/rspec best practices, migrate the current feature specs to be system specs, but keep them using rack_test (for now, maybe forever, tbd). There are not many of them and this will be straightforward. I have a WIP branch almost ready here.
  • Along with that, update the feature helpers and rspec configuration and so on to make sense as system specs. Mostly covered in that same branch, needs once over.
  • Do some consolidation around how various parts of the suite use database_cleaner and/or replace it with built-in capabilities where possible. Have not reviewed thoroughly, but suspect there's opportunity here.
  • Continue to migrate the api controller specs to request specs (not directly related to this change) ... and then on top of that, migrate many of the controller specs to be system specs, driven by rack_test where the JS interaction doesn't matter, and driven by js-enabled chromedriver/selenium where there's meaningful React/JS interaction to verify. Net result of that should be far fewer controller specs when all said and done ... and hopefully only ones wihch are effectively unit testing the controllers as ruby classes, not the request stack or browser interactions.

I know there are concerns about some combination of naming these things, keeping them separated from each other enough to run them appropriately, not need to spin up the whole streaming setup when not needed, etc -- and I believe these changes and that "roadmap" would more or less do that, but happy to consider anything I've missed. Hopefully it's easier to contemplate actual change proposal and not get hung up on terminology differences.

@renchap renchap added testing Automated lint and test suites ruby Pull requests that update Ruby code labels Nov 17, 2023
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@renchap
Copy link
Sponsor Member

renchap commented Jan 10, 2024

End-to-end testing is not only streaming server integration testing.

The end-to-end tests are intended to test the frontend using a real browser. This includes using the streaming server, but this is not the primary goal.

To me, end-to-end tests are system tests, ie tests that are using a stack as close as possible as a real deployment. They are done using a real browser, real database, and real server.

The other rack_test tests are not end-to-end tests, because they do not try to be as close as a deployment as possible, so, in my original intend, they should not be system tests, but something else (= "feature tests" like they were before).

End-to-end tests should always run in the same environment, and I dont think we should let them use the streaming server / real browser / … or not.

From what I understand, system tests in Rails are intended to be the end-to-end tests I described above. If you want to move the feature tests to system tests, then I would prefer having something like spec/end-to-end directory with the real end-to-end tests.

@renchap renchap mentioned this pull request Jan 10, 2024
@mjankowski
Copy link
Contributor Author

I agree with the spirit of that summary and don't want to get bogged down in the semantics/definitions. I will attempt to pull out just the capybara config update, and the "catch js errors" portions of this and the other PR, and we can continue to iterate past that.

I do think that a setup like this might ultimately make sense (whether it's done with tags or a dedicated dir which generates a new tag/type ... I don't really care), but I'm happy to get there one smaller step at a time instead of assuming it's correct before we move more things.

Will open new PRs on config/js stuff next.

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@mjankowski
Copy link
Contributor Author

Rebased this after the capybara config changed merged. Will rebase again after JS errors merge and see if anything useful left, or close.

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.84%. Comparing base (86500e3) to head (902f02f).
Report is 201 commits behind head on main.

❗ Current head 902f02f differs from pull request most recent head c8205eb. Consider uploading reports for the commit c8205eb to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #27932      +/-   ##
==========================================
- Coverage   85.01%   84.84%   -0.18%     
==========================================
  Files        1059     1038      -21     
  Lines       28277    28149     -128     
  Branches     4538     4536       -2     
==========================================
- Hits        24040    23883     -157     
- Misses       3074     3101      +27     
- Partials     1163     1165       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mjankowski mjankowski force-pushed the streaming-spec-configuration branch from 0d534e5 to c8205eb Compare March 1, 2024 17:55
@mjankowski mjankowski closed this Mar 13, 2024
@mjankowski mjankowski deleted the streaming-spec-configuration branch March 13, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ruby Pull requests that update Ruby code testing Automated lint and test suites
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants