Skip to content

test: fixtures module replaced fixturesDir #15936

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.

Sign up for GitHub

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

Closed
tpurcell wants to merge 3 commits into from
Closed

test: fixtures module replaced fixturesDir #15936

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47e30da
test: fixtures module replaced fixturesDir
tpurcell Oct 6, 2017
0950402
Merge branch 'master' of https://github.com/nodejs/node
tpurcell Oct 16, 2017
63f28fc
Merge branch 'master' of https://github.com/nodejs/node
tpurcell Oct 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions test/parallel/test-https-client-resume.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ if (!common.hasCrypto)
const assert = require('assert');
const https = require('https');
const tls = require('tls');
const fs = require('fs');
const fixtures = require('../common/fixtures');

const options = {
key: fs.readFileSync(`${common.fixturesDir}/keys/agent2-key.pem`),
cert: fs.readFileSync(`${common.fixturesDir}/keys/agent2-cert.pem`)
key: fixtures.readSync('keys/agent2-key.pem'),
cert: fixtures.readSync('keys/agent2-cert.pem')

@BridgeAR BridgeAR Oct 7, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use the readKey function instead.

@jasnell jasnell Oct 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance,

key: fixtures.readKey('agent2-key.pem'),
cert: fixtures.readKey('agent2-cert.pem')

@tpurcell tpurcell Oct 16, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to have gone dead on this. I'm obviously new to this and I've run into an issue.
When I submitted the PR all tests were passing. Today I:

  • Pulled from master to get my local repo up to date
  • Rebuilt node
  • Ran tests
git pull https://github.com/nodejs/node.git master
./configure 
make -j4
make test

I'm getting failed tests now:

ath: parallel/test-https-connect-address-family
/home/tpurcell/data/mycode/node/node/test/parallel/test-https-connect-address-family.js:39
    throw err;
    ^
Error: getaddrinfo EAI_AGAIN
    at Object._errnoException (util.js:1023:13)
    at errnoException (dns.js:63:15)
    at GetAddrInfoReqWrap.onlookupall [as oncomplete] (dns.js:112:26)
Command: out/Release/node /home/tpurcell/data/mycode/node/node/test/parallel/test-https-connect-address-family.js

I don't see a connection between this error and the code I should be changing. Like I said the tests ran clean before the pull from master and I've made no other local changes.

Questions:

  • Is there a way to find out the state of the current canonical build? If I could check that it would tell me if its just an issue with my local repo.
  • Am I making some perfectly obvious rookie mistake.

Thanks for the help

@joyeecheung joyeecheung Oct 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you have done some git merge on this branch, the best way to keep a PR up-to-date is to use git rebase instead (see the contributing guide)

On the error thrown by parallel/test-https-connect-address-family, that seems unrelated to this test so you could probabaly just ignore that . Judging from the message, looks like your machine timed out on resolving localhost to ::1, which that file should probably address by adding EAI_AGAIN on line 36..so yeah, this is probably an oversight of that test.

@tpurcell tpurcell Oct 17, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the response. After posting I did a completely fresh git clone of master and got the same errors so I figured it was something outside test-https-client-resume.

Thanks for the tip on rebase. I'll get my local checkout straight and then resubmit the PR.

Appreciate the help.

};

// create server
Expand Down