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

Refactored the vows test to use "macro"s and catch additional errors #674

Closed
wants to merge 2 commits into from

Conversation

bramp
Copy link

@bramp bramp commented Jan 31, 2021

Please ensure that your pull request fulfills these requirements:

  • The pull request is being made against the master branch
  • Tests for the changes have been added (for bug fixes / features)

What is the purpose of this pull request? (bug fix, enhancement, new feature,...)

Enhancement

What changes did you make?

Refactored the vows tests to:

  • Use respondsWithMessage "macro"s. This makes it easier to read, reduced code duplication, and tests for error conditions.
  • Check for errors while starting each server.
  • This should have no functional change, but made the tests shorter, and more correct. For example, I found that the server is not actually tested correctly for startup while working on another pull request.

Provide some example code that this change will affect, if applicable:

N/A

Is there anything you'd like reviewers to focus on?

Please provide testing instructions, if applicable:

npm test

bramp added 2 commits January 30, 2021 21:29
…or tests related changes:

* Every httpServer.createServer now has a vow to check for errors. Additionally, the server object is returned, instead of using the callbacks. This is because the server is an EventEmitter, and vows.js can capture the error correctly.

* Documented respondsWithMessage function

* Refactored one case of checking for a 404 with respondsWith(404, ...)
@bramp bramp changed the title Refactored the vows test to use a "macro". Refactored the vows test to use "macro"s and catch additional errors Jan 31, 2021
@thornjad
Copy link
Member

I like the idea of this PR! However, #693 moved us off of vows, so there's some significant changes. If you'd like to see the same changes in the new tests, I'd love a new PR!

@thornjad thornjad closed this Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants