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: replacing common.fixturesDir in test-require-exceptions.js #15860

Closed
wants to merge 2 commits into from

Conversation

tejbirsingh
Copy link
Contributor

Using common.fixtures module instead of common.fixturesDir based on task at Nodejs code and learn at Node.js Interactive 2017 in Vancouver.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

Using common.fixtures module instead of common.fixturesDir based on task at Nodejs code and learn at Node.js Interactive 2017 in Vancouver.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. 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
Copy link
Member

@gireeshpunathil gireeshpunathil left a comment

Choose a reason for hiding this comment

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

@@ -22,15 +22,16 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const fixtures = require ('../common/fixtures');
Copy link
Member

Choose a reason for hiding this comment

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

The linter complained:

not ok 25 - /usr/home/iojs/build/workspace/node-test-linter/test/parallel/test-require-exceptions.js
  ---
  message: Unexpected space between function name and paren.
  severity: error
  data:
    line: 25
    column: 18
    ruleId: func-call-spacing
  ...

Can you remove the space after require? Thanks.

Removing space after require to appease linter.
@tejbirsingh
Copy link
Contributor Author

@joyeecheung I have updated the PR based on your request. Thank you for letting me know.

@joyeecheung
Copy link
Member

@jasnell jasnell dismissed joyeecheung’s stale review October 16, 2017 04:56

The nit was corrected :-)

@lance
Copy link
Member

lance commented Oct 16, 2017

CI Results with unrelated failures.

6,pi1-raspbian-wheezy
+ git clean -fdx
warning: failed to remove out/Release/.nfs00000000000829ed000000b7
Removing config.gypi
Removing icu_config.gypi
Removing node
Removing out/Release/openssl-cli
Removing out/Release/node
Removing test.tap
Removing test/abort/testcfg.pyc
Removing test/addons-napi/testcfg.pyc
Removing test/addons/testcfg.pyc
Removing test/async-hooks/testcfg.pyc
Removing test/doctool/testcfg.pyc
Removing test/es-module/testcfg.pyc
Removing test/gc/testcfg.pyc
Removing test/inspector/testcfg.pyc
Removing test/internet/testcfg.pyc
Removing test/known_issues/testcfg.pyc
Removing test/message/testcfg.pyc
Removing test/parallel/testcfg.pyc
Removing test/pseudo-tty/testcfg.pyc
Removing test/pummel/testcfg.pyc
Removing test/sequential/testcfg.pyc
Removing test/testpy/__init__.pyc
Removing test/tick-processor/testcfg.pyc
Removing test/timers/testcfg.pyc
Removing test/tmp.0/
Removing tools/test.pyc
Removing tools/utils.pyc
Build step 'Execute shell' marked build as failure
TAP Reports Processing: START
Looking for TAP results report in workspace using pattern: *.tap
Did not find any matching files. Setting build result to FAILURE.
Checking ^not ok
aix61-ppc64
not ok 1896 inspector/test-stop-profile-after-done # TODO : Fix flaky test
  ---
  duration_ms: 0.906
  severity: flaky
  stack: |-
    [test] Connecting to a child Node process
    [test] Testing /json/list
    [err] Debugger listening on ws://127.0.0.1:34272/ded86544-6a2e-4776-aa90-fc9593cf608f
    [err] For help see https://nodejs.org/en/docs/inspector
    [err] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    { Error: connect ECONNREFUSED 127.0.0.1:34272
        at Object._errnoException (util.js:1023:13)
        at _exceptionWithHostPort (util.js:1044:20)
        at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1188:14)
      errno: 'ECONNREFUSED',
      code: 'ECONNREFUSED',
      syscall: 'connect',
      address: '127.0.0.1',
      port: 34272 }
    1
  ...
vs2017,win2016,0
not ok 468 inspector/test-bindings # TODO : Fix flaky test
  ---
  duration_ms: 0.184
  severity: crashed
  stack: |-
    oh no!
    exit code: CRASHED
  ...
not ok 469 inspector/test-debug-end # TODO : Fix flaky test
  ---
  duration_ms: 0.615
  severity: flaky
  stack: |-
    Test there's no crash stopping server that was not started
    Test there's no crash stopping server without connecting
    [err] Debugger listening on ws://127.0.0.1:50876/a065bb47-97c7-420e-be6f-ecb8498ebf0f
    [err] For help see https://nodejs.org/en/docs/inspector
    [err] 
    Test there's no crash stopping server after connecting
    [test] Connecting to a child Node process
    [test] Testing /json/list
    [err] Debugger listening on ws://127.0.0.1:50877/fa46e087-4253-435d-ab33-7e7d9a231f2c
    [err] For help see https://nodejs.org/en/docs/inspector
    [err] 
    [err] Debugger attached.
    [err] Debugger listening on ws://127.0.0.1:50880/8937680d-1980-47a5-8c15-8397c89f605c
    [err] For help see https://nodejs.org/en/docs/inspector
    [err] 
    { AssertionError [ERR_ASSERTION]: 42 === 3221225477
        at testSessionNoCrash (c:\workspace\node-test-binary-windows\test\inspector\test-debug-end.js:35:3)
        at 
        at process._tickCallback (internal/process/next_tick.js:188:7)
      generatedMessage: true,
      name: 'AssertionError [ERR_ASSERTION]',
      code: 'ERR_ASSERTION',
      actual: 42,
      expected: 3221225477,
      operator: '===' }
    1
  ...
---

This can be landed.

@lance lance self-assigned this Oct 16, 2017
@lance
Copy link
Member

lance commented Oct 16, 2017

Landed in: 9b3d6a0

@tejbirsingh thanks for the contribution!

@lance lance closed this Oct 16, 2017
lance pushed a commit that referenced this pull request Oct 16, 2017
Use common.fixtures module instead of common.fixturesDir based on task
at Nodejs code and learn at Node.js Interactive 2017 in Vancouver.

PR-URL: #15860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@tejbirsingh
Copy link
Contributor Author

@lance Yaay my first contribution!

targos pushed a commit that referenced this pull request Oct 18, 2017
Use common.fixtures module instead of common.fixturesDir based on task
at Nodejs code and learn at Node.js Interactive 2017 in Vancouver.

PR-URL: #15860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 18, 2017
Use common.fixtures module instead of common.fixturesDir based on task
at Nodejs code and learn at Node.js Interactive 2017 in Vancouver.

PR-URL: nodejs/node#15860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
Use common.fixtures module instead of common.fixturesDir based on task
at Nodejs code and learn at Node.js Interactive 2017 in Vancouver.

PR-URL: #15860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
Use common.fixtures module instead of common.fixturesDir based on task
at Nodejs code and learn at Node.js Interactive 2017 in Vancouver.

PR-URL: #15860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
Use common.fixtures module instead of common.fixturesDir based on task
at Nodejs code and learn at Node.js Interactive 2017 in Vancouver.

PR-URL: #15860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.