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 `ActiveSupport::Testing::FileFixtures`. #18658

Merged
merged 1 commit into from Jan 28, 2015
Merged

Conversation

@senny
Copy link
Member

senny commented Jan 23, 2015

It's a thin layer to provide easy access to sample files throughout
test-cases. This adds the directory test/fixtures/files to newly
generated applications.

This is something that came up in a discussion with @dhh

Most of our applications have helpers to access some kind of sample files. Possible use-cases are:

  • Test file uploads
  • Sample output to compare generated documents (.csv, .docx, .pdf, ...)
  • content used in test-cases like xml snippets.
  • ...
@senny
Copy link
Member Author

senny commented Jan 23, 2015

@carlosantoniodasilva @rafaelfranca @dhh interested to hear your thoughts.

The method names feel a bit awkward to me but it's the best I could come up with.

@senny senny added this to the 5.0.0 milestone Jan 23, 2015
@senny senny added the activesupport label Jan 23, 2015
@arthurnn
Copy link
Member

arthurnn commented Jan 23, 2015

🆒

@vipulnsward
Copy link
Member

vipulnsward commented Jan 23, 2015

Nice. @senny it would be awesome if this could be extended to support passing a file in controller requests in tests. Thoughts?

@rafaelfranca
Copy link
Member

rafaelfranca commented Jan 23, 2015

Seems good to me.

@robin850
robin850 reviewed Jan 25, 2015
View changes
activesupport/lib/active_support/testing/file_fixtures.rb Outdated
class_attribute :file_fixture_directory
end

# return the contents of the fixture file named +fixture_name+.

This comment has been minimized.

@robin850

robin850 Jan 25, 2015 Member

Nit-picky thing but it should be "Returns" here, no ?

@robin850
robin850 reviewed Jan 25, 2015
View changes
activesupport/lib/active_support/testing/file_fixtures.rb Outdated
file_fixture_path(fixture_name).read
end

# return the path to the fixture file named +fixture_name+.

This comment has been minimized.

@robin850

robin850 Jan 25, 2015 Member

Ditto.

@robin850
robin850 reviewed Jan 25, 2015
View changes
activesupport/lib/active_support/testing/file_fixtures.rb Outdated
extend ActiveSupport::Concern

included do
class_attribute :file_fixture_directory

This comment has been minimized.

@robin850

robin850 Jan 25, 2015 Member

There's no big deal but I guess we can also pluralize "fixture" here (so we are consistent with the default generated path), what do you think ? Otherwise, awesome addition, this was really something missing ! 👍

This comment has been minimized.

@senny

senny Jan 25, 2015 Author Member

@robin850 I'd rather keep it in sync with the fixture_path config option we have for AR fixtures:

test_help.rb

     self.fixture_path = "#{Rails.root}/test/fixtures/"
     self.file_fixture_directory = Rails.root + "test/fixtures/files"

This comment has been minimized.

@robin850

robin850 Jan 25, 2015 Member

Ah yes, fair point !

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jan 26, 2015 Member

Looking at the configuration I wonder if it'd not be confusing: fixture_path vs file_fixture_directory, as configs, considering there's also the file_fixture_path method. Maybe not?

This comment has been minimized.

@senny

senny Jan 26, 2015 Author Member

I intentionally chose a different name because there is a file_fixture_path method. Thought it would only make it harder to hold them apart...

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jan 26, 2015 Member

Yes, just a bit worried about the consistency between the two, without looking the internals I'd think file_fixture_path would be a config like fixture_path is. Maybe it's not that big deal, just raising the point.

This comment has been minimized.

@senny

senny Jan 26, 2015 Author Member

@carlosantoniodasilva I'm fine with using file_fixture_path for the config option but I think we need a different name for the helper then.

@senny senny force-pushed the senny:file_fixtures branch Jan 25, 2015
@carlosantoniodasilva
carlosantoniodasilva reviewed Jan 26, 2015
View changes
railties/lib/rails/test_help.rb Outdated
@@ -21,6 +21,7 @@
class ActiveSupport::TestCase
include ActiveRecord::TestFixtures
self.fixture_path = "#{Rails.root}/test/fixtures/"
self.file_fixture_directory = Rails.root + "test/fixtures/files"

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jan 26, 2015 Member

How about: fixture_path + "files"?

This comment has been minimized.

@senny

senny Jan 26, 2015 Author Member

👍

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Jan 26, 2015

I wonder if we should touch fixture_file_upload to make use of this new files directory rather than the normal fixtures_path in this case?

Other than that it seems good to me, good to have something simple builtin to help with that.

@senny senny force-pushed the senny:file_fixtures branch 2 times, most recently Jan 26, 2015
@senny
Copy link
Member Author

senny commented Jan 28, 2015

I changed the configuration option to file_fixture_path and the methods to file_fixture and file_fixture_content.

@carlosantoniodasilva what do you think?

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Jan 28, 2015

Cool, I like it better 👍, do you prefer like this?

Would you think that leaving the "read" logic for the developers wouldn't be fine? I mean, we could use file_fixture('zomg.txt').read directly on the tests, and only introduce this helper method with the config.

@senny
Copy link
Member Author

senny commented Jan 28, 2015

@carlosantoniodasilva 👍 I think we can get rid of _content and delegate .read to the user. I'll try to cover it in the documentation. Once that base-layer is merged we can start talking about fixture_file_upload.

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Jan 28, 2015

Alright, great 👍 ❤️

@senny senny force-pushed the senny:file_fixtures branch Jan 28, 2015
@senny
Copy link
Member Author

senny commented Jan 28, 2015

@carlosantoniodasilva mind a final 👀?

@carlosantoniodasilva
carlosantoniodasilva reviewed Jan 28, 2015
View changes
activesupport/CHANGELOG.md Outdated
@@ -1,3 +1,11 @@
* Add `file_fixture` to `ActiveSupport::TestCase`.
They provide a simple mechanism to access sample files in your test cases.

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jan 28, 2015 Member

s/they provide/it provides ?

@carlosantoniodasilva
carlosantoniodasilva reviewed Jan 28, 2015
View changes
activesupport/lib/active_support/testing/file_fixtures.rb Outdated

# Returns a +Pathname+ to the fixture file named +fixture_name+.
#
# raises ArgumentError if +fixture_name+ can't be found.

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jan 28, 2015 Member

Should it be capitalized Raises?

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Jan 28, 2015

💚💛❤️💙💜

It's a thin layer to provide easy access to sample files throughout
test-cases. This adds the directory `test/fixtures/files` to newly
generated applications.
@senny senny force-pushed the senny:file_fixtures branch to d28e5b9 Jan 28, 2015
senny added a commit that referenced this pull request Jan 28, 2015
Add `ActiveSupport::Testing::FileFixtures`.
@senny senny merged commit 58047eb into rails:master Jan 28, 2015
1 check was pending
1 check was pending
continuous-integration/travis-ci The Travis CI build is in progress
Details
@senny senny deleted the senny:file_fixtures branch Jan 28, 2015
@senny
Copy link
Member Author

senny commented Jan 28, 2015

@carlosantoniodasilva thanks man!!! 💛

senny added a commit that referenced this pull request Jan 29, 2015
@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Jan 29, 2015

thank you 👍 💙

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.