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

Overhaul Spec Tests #576

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

Overhaul Spec Tests #576

wants to merge 41 commits into from

Conversation

MatthewKennedy
Copy link
Contributor

@MatthewKennedy MatthewKennedy commented Dec 26, 2022

  • Ensure locale is set back to :en after tests that change the locale to :fr.
  • Rename log_out to log_out_via_frontend_user_menu to avoid tests on admin side using it by mistake.
  • Update some old tests to modern Rspec syntax.
  • Lint some files (mostly to re-run tests).

Fix Admin Sign Out spec, it was not testing the sign out from the admin side, rather it was logging in, redirecting to the frontend and logging back out again.

Now creates an admin user, logs into the admin, redirects to the admin as expected, and then logs out from the admin UI

@MatthewKennedy MatthewKennedy changed the title Fix failing Specs Fix Failing Specs Dec 27, 2022
@MatthewKennedy MatthewKennedy changed the title Fix Failing Specs Overhaul Spec Tests Dec 28, 2022
Copy link
Member

@rafalcymerys rafalcymerys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR @MatthewKennedy , thanks for that! 🙌
I just have one comment regarding one method that you removed - I think it should stay as it is, but I'll need your feedback on that.

@@ -60,10 +60,6 @@ def self.frontend_available?
@@frontend_available ||= Gem::Specification.find_all_by_name('spree_frontend').any?
end

def self.api_available?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is safe to be removed, it's used e.g. in lib/spree/auth/engine.rb:43, to determine whether to load decorators for API classes.

@@ -31,7 +31,7 @@ def self.activate
Rails.configuration.cache_classes ? require(c) : load(c)
end
if Spree::Auth::Engine.backend_available?
Dir.glob(File.join(File.dirname(__FILE__), "../../controllers/backend/*/*/*_decorator*.rb")) do |c|
Dir.glob(File.join(File.dirname(__FILE__), "../../controllers/backend/**/*_decorator*.rb")) do |c|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch :)

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

Successfully merging this pull request may close these issues.

2 participants