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 a test for create_post in import scripts #18893

Merged
merged 1 commit into from Jan 31, 2023

Conversation

harry-wood
Copy link
Contributor

@harry-wood harry-wood commented Nov 5, 2022

Add some testing of the create_post method in ImportScripts::Base

Basic test of Post creation and (if enabled) the bbcode_to_md call.

@CLAassistant
Copy link

CLAassistant commented Nov 5, 2022

CLA assistant check
All committers have signed the CLA.

}.to change{Post.count}.by(1)
end

if ENV["IMPORT"] == "1"
Copy link
Contributor Author

@harry-wood harry-wood Nov 5, 2022

Choose a reason for hiding this comment

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

It's a little unconventional to do conditional testing like this (developers would need to prefix their rspec command with IMPORT=1 to activate these tests) but then it's a little unconventional to do conditional gem includes in Gemfile.

Arguably it would be more flexible if the condition was String.method_defined?(:bbcode_to_md) (Then developers need only have the gem installed. No need to set IMPORT=1). Let me know if you'd prefer that, but I decided it would look even more unclear as to why these tests are only sometimes run. As it stands, the first test here serves to document the unusual conditional Gemfile set-up.

Add some testing of the `create_post` method in ImportScripts::Base

Basic test of Post creation and (if enabled) the bbcode_to_md call.
@harry-wood
Copy link
Contributor Author

harry-wood commented Nov 9, 2022

This one is ready for review @gschlager (Putting this test in place is the first step of the bigger WIP PR you were looking at)

@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/migration-from-fluxbb-while-preserving-incoming-links/172351/8

@gschlager gschlager merged commit bdf8815 into discourse:main Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants