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

Add hapi support #8

Closed
rvagg opened this issue Aug 17, 2015 · 18 comments
Closed

Add hapi support #8

rvagg opened this issue Aug 17, 2015 · 18 comments

Comments

@rvagg
Copy link
Member

rvagg commented Aug 17, 2015

Moving discussion from https://github.com/rvagg/iojs-smoke-tests/issues/1

I'm +1 on adding Hapi given its heavy test burden and enterprise popularity. Does it need anything more than an entry on the README?

@jasnell
Copy link
Member

jasnell commented Aug 17, 2015

If hapi supports npm test out of the box on what is installed from npm install, then no, nothing else is required. If, it doesn't however, you'll want to add an entry to https://github.com/nodejs/citgm/blob/master/lib/lookup.json and make sure to specify the -l switch when running the hapi test

@cjihrig
Copy link

cjihrig commented Aug 23, 2015

All you need is npm install and npm test.

@MylesBorins
Copy link
Contributor

I am unsure if this is a regression but the current version of hapi has a single failing test. Will investigate.

@MylesBorins
Copy link
Contributor

So there is a single failed test with hapi when running within citgm

Not sure what is causing this weirdness, but it is worth exploring. I ran the test suite for v11.1.2 in both citgm and bash, and all tests passed in bash.

verbose: npm-test:           | Failed tests:
verbose:                     |
verbose:                     | 70) Connection creates a server listening on a
verbose:                     | unix domain socket:
verbose:                     |
verbose:                     | listen EADDRINUSE
verbose:                     | /private/var/folders/ty/q7q6b07j5r3c7nvnpzm6hkq40…
verbose:                     | 000gn/T/422862c8-9a51-424c-b377-f22ecac239c0/hapi…
verbose:                     | /test/hapi-server.socket
verbose:                     |
verbose:                     | at Object.exports._errnoException (util.js:855:11

@rvagg
Copy link
Member Author

rvagg commented Dec 17, 2015

tbh I've had a lot of trouble with Hapi's test suite, it fails inconsistently across all versions of Node.js and Hapi. I've been (a) taking failures with a grain of salt and/or (b) comparing a smoke test run with Hapi to a previous release of Node.js to see if the same failure occurs there to ensure the failure is not new.

@MylesBorins
Copy link
Contributor

Closing for now... can revisit if hapi wants support

@AdriVanHoudt
Copy link

@cjihrig brought this back up and I think it would be valuable to add hapi to citgm.
I think it meets all the requirements.

@gibfahn
Copy link
Member

gibfahn commented Jun 2, 2017

I think it would be valuable to add hapi, but I've been unable to get the tests to pass reliably on my machine.

wget https://github.com/hapijs/hapi/archive/v16.3.0.tar.gz
tar -xf v16.3.0.tar.gz
npm it
Failed tests:

  718) transmission marshal() closes file handlers when not reading file stream:

      actual expected

      200304

      Expected 200 to equal specified value

      at server.inject (/Users/gib/tmp/hapi/hapi-16.3.0/test/transmit.js:96:48

  772) transmission transmit() does not open file stream on 304:

      Timed out (3000ms) - transmission transmit() does not open file stream on 304


  780) transmission transmit() does not leak stream data when request aborts before stream is returned:

      Timed out (3000ms) - transmission transmit() does not leak stream data when request aborts before stream is returned



3 of 897 tests failed
Test duration: 14288 ms
Assertions count: 2238 (verbosity: 2.49)
No global variable leaks detected
Coverage: 99.98% (1/4225)
lib/response.js missing coverage on line(s): 633
Code coverage below threshold: 99.98 < 100
Linting results: No issues

@AdriVanHoudt
Copy link

AdriVanHoudt commented Jun 2, 2017

I could reproduce, I opened an issue hapijs/hapi#3508

@AdriVanHoudt
Copy link

It seems a PR with a fix is there hapijs/hapi#3507
@gibfahn could you try that branch?

@hueniverse
Copy link

This seems like the wrong attitude coming from people responsible for maintaining the health of the ecosystem as a whole (not to mention the fact that no one bothered to actually connect with me to address any concerns). Including hapi in the node smoke tests adds more value to node than to hapi. We test hapi extensively and we find all the issues when there are new versions of node. By excluding one of the most extensive and deepest test suite here, you are doing the node community as a whole a disservice. For many years, hapi was the only framework to surface bugs in new versions of node.

I am always open to improving the test suite for hapi and to collaborate with the node core team. Every single one of you knows this very well which is why this thread is so annoying.

@MylesBorins MylesBorins reopened this Jun 2, 2017
@MylesBorins
Copy link
Contributor

@hueniverse to be honest this thread was closed before CITGM really had hit a stride and was primarily an experiment. The fact that hapi has fallen to the wayside is definitely an oversight

I'm reopening this so we can keep track of the status. This is definitely a priority

@gibfahn
Copy link
Member

gibfahn commented Jun 2, 2017

to be honest this thread was closed before CITGM really had hit a stride and was primarily an experiment.

Yeah this was from 2015, didn't even know CitGM was around then!

I am always open to improving the test suite for hapi and to collaborate with the node core team.

Great, we'll get it added to CitGM. As long as if there's a failure we can work with you to get it fixed (assuming it's not a Node core issue) then it should be smooth sailing.

@gibfahn
Copy link
Member

gibfahn commented Jun 2, 2017

PR: #436

gibfahn added a commit to gibfahn/citgm that referenced this issue Jun 2, 2017
gibfahn added a commit to gibfahn/citgm that referenced this issue Jun 2, 2017
@DavidTPate
Copy link

I'll keep a lookout for issues related to CiTGM with Hapi as well.

@gibfahn
Copy link
Member

gibfahn commented Jun 2, 2017

Thanks @DavidTPate.

So CitGM has a maintainers field, which is a list of people we ping if there's an issue with a module and we need some expert help.

I've included @hueniverse in #436, if anyone else is willing to be included in there let us know (maybe @AdriVanHoudt or @DavidTPate ?)

@DavidTPate
Copy link

DavidTPate commented Jun 2, 2017

@gibfahn I'm willing to volunteer for those notifications as well. I know that Eran is a very busy person these days.

gibfahn added a commit to gibfahn/citgm that referenced this issue Jun 2, 2017
@AdriVanHoudt
Copy link

@gibfahn you can add me no problem

gibfahn added a commit to gibfahn/citgm that referenced this issue Jun 3, 2017
@devinivy devinivy mentioned this issue Dec 28, 2020
1 task
targos pushed a commit to gibfahn/citgm that referenced this issue Dec 28, 2020
targos pushed a commit to gibfahn/citgm that referenced this issue Dec 30, 2020
@targos targos closed this as completed in d170bd6 Mar 16, 2021
MylesBorins pushed a commit that referenced this issue Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants