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

Add RSpec 4 compatibility #17652

Merged
merged 7 commits into from Jul 28, 2022
Merged

Conversation

pirj
Copy link
Contributor

@pirj pirj commented Jul 25, 2022

You may check a PR that also switches to use RSpec 4 and RSpec-Rails 6: pirj#1

There are a few reasons it doesn't make sense to switch right away:

What's Done

Comply to strict predicate matchers

See:

Basically,

expect(OpenStruct.new(foo?: nil)).to_not be_foo

won't pass anymore, as strict predicate matchers would only accept true/false, but not "truthy" or "falsey" values.

Use the non-globally exposed RSpec syntax

See rspec/rspec-core#2803

describe/shared_examples won't work without the explicit RSpec..
should/should_not were deprecate nearly a decade ago and will be removed.

Misc

Discourse specs use defaults, and it helped a lot, there's just --color option that had to be removed from .rspec file.
rspec/rspec-core@0407831

RSpec-Rails 6

No changes turned out to be necessary.

@CLAassistant
Copy link

CLAassistant commented Jul 25, 2022

CLA assistant check
All committers have signed the CLA.

@@ -254,7 +254,7 @@
theme.set_field(target: :common, name: SvgSprite.theme_sprite_variable_name, upload_id: upload.id, type: :theme_upload_var)
theme.save!

expect(Upload.where(id: upload.id)).to be_exist
expect(Upload.where(id: upload.id)).to be_exists
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because Upload.where(...).exist? method doesn't exist, but exists? does.

spec/models/topic_spec.rb Outdated Show resolved Hide resolved
spec/models/topic_spec.rb Outdated Show resolved Hide resolved
spec/models/topic_spec.rb Outdated Show resolved Hide resolved
spec/models/topic_spec.rb Outdated Show resolved Hide resolved
@tgxworld
Copy link
Contributor

Thank you @pirj, some comments but otherwise the PR looks good 👍

@pirj
Copy link
Contributor Author

pirj commented Jul 26, 2022

Thanks for the quick review, @tgxworld . Do you want me to squash the last commit?

@pirj pirj force-pushed the add-rspec-4-compatibitility branch from d81a86c to 677e3eb Compare July 26, 2022 20:45
@pirj

This comment was marked as outdated.

@tgxworld
Copy link
Contributor

Don't worry about squashing because we'll do a squash and merge once the PR is ready.

@pirj pirj force-pushed the add-rspec-4-compatibitility branch from 52eb53c to 8e27544 Compare July 27, 2022 10:56
@pirj pirj force-pushed the add-rspec-4-compatibitility branch from 8e27544 to d5443c1 Compare July 27, 2022 10:58
@pirj
Copy link
Contributor Author

pirj commented Jul 27, 2022

the PR is ready

Is there something I can do to help making it ready?

@tgxworld tgxworld merged commit 493d437 into discourse:main Jul 28, 2022
@tgxworld
Copy link
Contributor

@pirj Thank you for your contribution here 👍

@pirj pirj deleted the add-rspec-4-compatibitility branch July 28, 2022 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants