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

Fix issue with coverage report. #262

Merged
merged 1 commit into from
May 27, 2020
Merged

Fix issue with coverage report. #262

merged 1 commit into from
May 27, 2020

Conversation

rwjblue
Copy link
Collaborator

@rwjblue rwjblue commented May 27, 2020

Unfortunately, this codepath is not under test and the changes from #251 left it in an invalid state.

  • The REQUEST_ASYNC variable was no longer present, and therefore referencing it threw an error
  • The guard around xhr.responseText was no longer needed (we only ever get JSON, and xhr.response is already parsed properly for us)

We ultimately need to refactor to get this coverage reporting under test (plausibly an end-to-end acceptance test that confirms the expected JSON reporter output after running all the tests).

@rwjblue rwjblue added the bug label May 27, 2020
@rwjblue rwjblue force-pushed the fix-coverage-reporting branch from 8dd0676 to 83887fb Compare May 27, 2020 20:03
@@ -12,7 +12,7 @@
var data = JSON.stringify(coverageData || {});

var request = new XMLHttpRequest();
request.open('POST', '/write-coverage', REQUEST_ASYNC);
request.open('POST', '/write-coverage');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

XHR's .open method defaults to async if not given a value of false for the 3rd argument.

https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/open

Unfortunately, this codepath is not under test and the changes from a
prior PR left it in an invalid state.

* The REQUEST_ASYNC variable was no longer present, and therefore
  referencing it threw an error
* The guard around `xhr.responseText` was no longer needed (we only ever
  get `JSON`, and `xhr.response` is already parsed properly for us)

We ultimately need to refactor to get this coverage reporting under test
(plausibly an end-to-end acceptance test that confirms the expected JSON
reporter output after running all the tests).
@rwjblue rwjblue force-pushed the fix-coverage-reporting branch from 83887fb to cc22041 Compare May 27, 2020 20:21
@rwjblue rwjblue merged commit dde740c into master May 27, 2020
@rwjblue rwjblue deleted the fix-coverage-reporting branch May 27, 2020 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants