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

WP-5202 Switch mock-aware requests to real requests before finalizing them #290

Merged
merged 3 commits into from
Sep 25, 2017

Conversation

evanweible-wf
Copy link
Contributor

Problem

When MockTransports are installed with fallThrough enabled, we check to see if each request is mock-aware and if it would be handled by an expectation or a mock handler. If it wouldn't be, we switch it to a real request so that it doesn't get stuck (i.e. "fall-through").

Currently, we finalize the request prior to making this check. It turns out that this can have an adverse effect on MultipartRequests due to their divergent browser and mock implementations. In the browser, the MultipartRequest files map accepts dart:html.File and dart:html.Blob instances, but those types cannot be imported in any platform other than the browser. As a result, attempting to finalize the mock-aware MultipartRequest will fail if either a File or Blob instance is attached because it does not recognize them.

Solution

The solution is to switch to a real request (if applicable) prior to finalizing the request, which will avoid the issue altogether if the request isn't destined to be handled by a mock handler or expectation anyway.

Testing

  • CI passes (applicable test has been updated – you can verify by checking out 76e6488 and running the tests without the fix)

Code Review

@Workiva/web-platform-pp

@aviary-wf
Copy link

aviary-wf commented Sep 22, 2017

Raven

Number of Findings: 0

@rmconsole3-wf rmconsole3-wf changed the title Switch mock-aware requests to real requests before finalizing them WP-5202 Switch mock-aware requests to real requests before finalizing them Sep 22, 2017
@codecov-io
Copy link

Codecov Report

Merging #290 into master will not change coverage.
The diff coverage is 100%.

@jayudey-wf
Copy link
Contributor

jayudey-wf commented Sep 25, 2017

QA +1

  • Testing instruction
  • Dev +1's
  • Dev/QA +10 with detail of what was tested
    • verified that test was appropriately failing without the corresponding source code update
  • Security review (if required)
  • Unit tests created/updated
  • All unit tests pass
  • Rosie has run and reports properly the release the ticket will be included in

Merging into master.

@jayudey-wf jayudey-wf merged commit ee638e0 into master Sep 25, 2017
@jayudey-wf jayudey-wf deleted the switch_to_real_request_before_finalizing branch September 25, 2017 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants