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
Upgrade tap #745
Upgrade tap #745
Conversation
fix deprecated test methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it! Just a package-lock conflict to solve!
Fixed package-lock |
Haha i think your package-lock fix from #744 is conflicting here again |
It's the never-ending struggle, should be fixed now |
Not sure why some tests are passing whilst others are failing based on OS and node js versions... |
Yeah we've been having some issues with non-deterministic errors, I made them a little better recently but they still pop up. Check out #723 if you have any insight. For now I'm just re-running them to see if that works |
For some reason this PR is just not letting tests pass, the same non-deterministic errors are happening every time, where normally it's only ever 10 runs or so on master or other PRs, so maybe something in this new tap version is triggering them harder. I (or anyone who jumps in first) am going to take a look at the tests over the next few days and see what I can do, and I'm going to let this PR just sit until then, because I don't want to make the tests harder in master. If you're doing this for Hacktoberfest I can do a one-off acceptance so you still get credit, just let me know, otherwise this may sit for a bit. |
@thornjad - I don't need any more Hacktoberfest approvals, so don't worry about that; I agree that you shouldn't merge this until we at least figure out why the tests are failing. In the meanwhile I'll see if I can reproduce the issues on macOS with older NodeJS as that seems to be the most common culprit. Might be relating to clean-up of resources if it's a port in use, perhaps Tap updates are no longer cleaning up like they used to or somehow exacerbating an existing issue. |
Apologies, I lost track of this and feel it's probably or of date enough to forget about now. |
Update tap and fix deprecated test methods
What changes did you make?
Upgraded to latest tap and replaced deprecated test methods in the unit tests.
Provide some example code that this change will affect, if applicable:
Is there anything you'd like reviewers to focus on?
Hopefully the changes are pretty straightforward, but note that whilst test coverage remains the same the newer version of tap reports anything less than 100% coverage as an error by default:
To avoid this I have added the
no-check-coverage
flag to the npm scripts inside package.json.Please ensure that your pull request fulfills these requirements:
master
branchhttp-server --help
textWhat is the purpose of this pull request? (bug fix, enhancement, new feature,...)
enhancement and security fixes (dependent packages)