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

Upgrade tap #745

Closed
wants to merge 4 commits into from
Closed

Conversation

chris--jones
Copy link
Contributor

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:

  • The pull request is being made against the master branch
  • 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)

@thornjad thornjad added the patch version Small, non-breaking, bug fix or trivial change label Oct 13, 2021
Copy link
Member

@thornjad thornjad left a 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!

@thornjad thornjad added the dependencies Pull requests that update a dependency file label Oct 13, 2021
@chris--jones
Copy link
Contributor Author

Fixed package-lock

@thornjad
Copy link
Member

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

@chris--jones
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@thornjad
Copy link
Member

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
Copy link
Member

thornjad commented Oct 18, 2021

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.

@chris--jones
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file patch version Small, non-breaking, bug fix or trivial change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants