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

test: fixtures module replaced fixturesDir #15936

Closed
wants to merge 3 commits into from

Conversation

tpurcell
Copy link
Contributor

@tpurcell tpurcell commented Oct 6, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@mscdex mscdex added the https Issues or PRs related to the https subsystem. label Oct 6, 2017
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
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')
Copy link
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.

Copy link
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')

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

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

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

@joyeecheung
Copy link
Member

Ping @tpurcell

@joyeecheung
Copy link
Member

Superseded by #16262

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants