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.

Sign up for GitHub

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

Jump to bottom

[Rails 5][#5117] Fix file upload spec #5153

Merged
merged 2 commits into from Apr 11, 2019
Merged

[Rails 5][#5117] Fix file upload spec #5153

merged 2 commits into from Apr 11, 2019

Conversation

lizconlan
Copy link

@lizconlan lizconlan commented Apr 2, 2019 •
edited

Relevant issue(s)

Fixes #5117

What does this do?

Moves the file upload spec that fails under Rails 5 (the change in parameters means the file upload handling works differently now) to be an integration spec that can do an end-to-end check instead of trying to intercept and override the passed file object.

Why was this needed?

The old spec was breaking under Rails 5...

AdminPublicBodyController GET #import_csv when handling a POST request if there is a csv file param should try to get the contents and original name of a csv file param
Failure/Error: expect(@file_object).to receive(:read).and_return('some contents')

  (#<Rack::Test::UploadedFile:0x000000000e7aeb18 @content_type=nil, @original_filename="fake-authority-type.csv", @tempfile=#<Tempfile:/tmp/fake-authority-type.csv20190222-20042-13c5ezf>>).read(*(any args))
      expected: 1 time with any arguments
      received: 0 times with any arguments
# ./spec/controllers/admin_public_body_controller_spec.rb:729:in `block (5 levels) in <top (required)>'

@lizconlan lizconlan force-pushed the rails5/5117-fix-file-upload-spec branch 2 times, most recently from 29aaa27 to 6038d33 Compare April 2, 2019 20:45
gbp
gbp requested changes Apr 11, 2019
View reviewed changes
Copy link
Member

@gbp gbp left a comment

Choose a reason for hiding this comment

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

Could you add a commit message to 6038d33 explaining why has been changed to an integration spec.

@lizconlan lizconlan changed the title [Rails5][#5117] Fix file upload spec [Rails 5][#5117] Fix file upload spec Apr 11, 2019
@lizconlan
Make checking the file upload function an integration spec
357199f 
As what we're trying to check here is that the uploaded file is accessed
and the contents used to create a preview of the information we're about
to load into the database when we press the "Dry run" button, this makes
more sense as an integration spec.

Also with the Rails 5 changes in handling parameters, the original spec
fails as it's harder to intercept the actual file object as it's no
longer a case of passing it in as a param. And our original stubbed the
file read in any case so was only testing that `read` was called, this
replacement checks that the content appears on screen as expected.
@lizconlan lizconlan force-pushed the rails5/5117-fix-file-upload-spec branch from 6038d33 to 357199f Compare April 11, 2019 11:24
@lizconlan lizconlan assigned gbp and unassigned lizconlan Apr 11, 2019
@lizconlan
Copy link
Author

Done. Sorry! (Left a handwaving description in the PR and missed the one on the commit itself)

@gbp gbp removed their assignment Apr 11, 2019
@lizconlan lizconlan merged commit 357199f into develop Apr 11, 2019
@lizconlan lizconlan deleted the rails5/5117-fix-file-upload-spec branch June 12, 2019 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbp gbp gbp approved these changes

Assignees

@lizconlan lizconlan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@lizconlan @gbp