http-server icon indicating copy to clipboard operation
http-server copied to clipboard

Upgrade tap

Open chris--jones opened this issue 3 years ago • 7 comments

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:

    request.get(`http://localhost:${port}/a.txt`, (err, res) => {
-      t.ifError(err);
+      t.error(err);
      t.equal(res.statusCode, 200, 'a.txt should be found');
      t.equal(res.headers['cache-control'], 'max-whatever=3600');
            requestAsync("http://localhost:8080/").then(res => {
              t.ok(res);
              t.equal(res.statusCode, 200);
-              t.includes(res.body, './file');
-              t.includes(res.body, './canYouSeeMe');
+              t.match(res.body, './file');
+              t.match(res.body, './canYouSeeMe');

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:

ERROR: Coverage for lines (87.02%) does not meet global threshold (100%)
ERROR: Coverage for functions (89.29%) does not meet global threshold (100%)
ERROR: Coverage for branches (77.53%) does not meet global threshold (100%)
ERROR: Coverage for statements (86.66%) does not meet global threshold (100%)

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:

  • [X] The pull request is being made against the master branch
  • [X] Tests for the changes have been added (for bug fixes / features)
  • [ ] New features/options have been documented in:
    • [ ] http-server --help text
    • [ ] README.md
    • [ ] doc/http-server.1

What is the purpose of this pull request? (bug fix, enhancement, new feature,...) enhancement and security fixes (dependent packages)

chris--jones avatar Oct 09 '21 03:10 chris--jones

Fixed package-lock

chris--jones avatar Oct 15 '21 08:10 chris--jones

Haha i think your package-lock fix from #744 is conflicting here again

thornjad avatar Oct 15 '21 15:10 thornjad

Haha i think your package-lock fix from #744 is conflicting here again

It's the never-ending struggle, should be fixed now

chris--jones avatar Oct 16 '21 00:10 chris--jones

Not sure why some tests are passing whilst others are failing based on OS and node js versions...

chris--jones avatar Oct 16 '21 00:10 chris--jones

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

thornjad avatar Oct 18 '21 14:10 thornjad

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 avatar Oct 18 '21 15:10 thornjad

@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.

chris--jones avatar Oct 19 '21 02:10 chris--jones

Apologies, I lost track of this and feel it's probably or of date enough to forget about now.

chris--jones avatar Oct 08 '22 05:10 chris--jones