Skip to content

test: Added more validations to setEncoding - #9997

Closed
pauljlucas wants to merge 2 commits into
nodejs:masterfrom
pauljlucas:paul/updating-setencoding-test
Closed

test: Added more validations to setEncoding#9997
pauljlucas wants to merge 2 commits into
nodejs:masterfrom
pauljlucas:paul/updating-setencoding-test

Conversation

@pauljlucas

@pauljlucaspauljlucas commented Dec 1, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • [x ] make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • [x ] commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

  • Added more strict validations to properly test setEncoding

  • Changed all var usage to const or let

  • Changed assert.equal to assert.strictEqual

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@imyllerimyller added code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. tls Issues and PRs related to the tls subsystem. labels Dec 1, 2016
@gibfahn

gibfahn commented Dec 1, 2016

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/5050/

@bnoordhuis

bnoordhuis commented Dec 1, 2016

Copy link
Copy Markdown
Member

Can you fix the lint errors?

@pauljlucas
test: Added more validations to setEncoding
5066e27
- Added more strict validations to properly test setEncoding
- Changed all var usage to const or let
- Changed assert.equal to assert.strictEqual
@pauljlucas
pauljlucasforce-pushed the paul/updating-setencoding-test branch from 309a854 to 5066e27CompareDecember 1, 2016 22:00
@pauljlucas

pauljlucas commented Dec 1, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Hey @gibfahn and @bnoordhuis,

Figured out the issue and fixed. Looks like my comments went on too long :)

Let me know if you have anything else that you would like me to change and thanks again for the workshop today.

@gibfahn

gibfahn commented Dec 2, 2016

Copy link
Copy Markdown
Member

CI 2: https://ci.nodejs.org/job/node-test-pull-request/5102/

@pauljlucas

pauljlucas commented Dec 2, 2016

Copy link
Copy Markdown
ContributorAuthor

Hey @gibfahn

Just looked at the failed linux portion and am seeing that the test i modified passes:

ok 1078 parallel/test-tls-set-encoding
---
duration_ms: 0.265
...

from CI console output: https://ci.nodejs.org/job/node-test-commit-linux/6374/nodes=ubuntu1604_docker_alpine34-64/console

Not sure if this is something on the code side or jenkins side, but if you have any ideas would appreciate you pointing me in the right direction to get this working. Thanks

@gibfahn

gibfahn commented Dec 2, 2016•
edited
Loading

Copy link
Copy Markdown
Member

@pauljlucas Looks like a flaky test. As the test you modified passed on all platforms, I'd say CI was successful.

We could rerun, but I don't think it's necessary.

@pauljlucas

pauljlucas commented Dec 2, 2016

Copy link
Copy Markdown
ContributorAuthor

Thanks for the clarification @gibfahn. I'm fine with whatever the standard is. Let me know if any clarifications are needed or if there is anything i need to do for getting the code reviewed.

cjihrig
cjihrig requested changes Dec 3, 2016
View reviewed changes
Comment threadtest/parallel/test-tls-set-encoding.js Outdated

// Confirming the buffer string is encoded in ASCII
// and thus does NOT match the UTF8 string
assert.notEqual(buffer, messageUtf8);

@cjihrigcjihrigDec 3, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you use assert.notStrictEqual() here.

@pauljlucas
test: Adding notStrictEqual to setencoding test
d421e32
This is changing a notEqual test to notStrictEqual
as reference in pr: nodejs#9997
@pauljlucas

pauljlucas commented Dec 4, 2016

Copy link
Copy Markdown
ContributorAuthor

@cjihrig Change made in commit d421e32.

cjihrig
cjihrig approved these changes Dec 4, 2016
View reviewed changes

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/5164/

jasnell pushed a commit that referenced this pull request Dec 5, 2016
@pauljlucas@jasnell
test: Added more validations to setEncoding
0c45959
* Added more strict validations to properly test setEncoding
* Changed all var usage to const or let
* Changed assert.equal to assert.strictEqual
PR-URL: #9997
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

jasnell commented Dec 5, 2016

Copy link
Copy Markdown
Member

Landed in 0c45959. Thank you for the PR and for participating in code-and-learn!

@jasnelljasnell closed this Dec 5, 2016
Fishrock123 pushed a commit that referenced this pull request Dec 6, 2016
@pauljlucas@Fishrock123
test: Added more validations to setEncoding
e8fc7fc
* Added more strict validations to properly test setEncoding
* Changed all var usage to const or let
* Changed assert.equal to assert.strictEqual
PR-URL: #9997
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Dec 8, 2016
@pauljlucas@addaleax
test: Added more validations to setEncoding
eb61d91
* Added more strict validations to properly test setEncoding
* Changed all var usage to const or let
* Changed assert.equal to assert.strictEqual
PR-URL: nodejs#9997
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
@pauljlucasJonathan Darling
test: Added more validations to setEncoding
8d16d31
* Added more strict validations to properly test setEncoding
* Changed all var usage to const or let
* Changed assert.equal to assert.strictEqual
PR-URL: nodejs#9997
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@pauljlucas@MylesBorins
test: Added more validations to setEncoding
fc5fda5
* Added more strict validations to properly test setEncoding
* Changed all var usage to const or let
* Changed assert.equal to assert.strictEqual
PR-URL: #9997
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Dec 21, 2016
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@cjihrigcjihrigcjihrig approved these changes
+1 more reviewer
@gibfahngibfahngibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

code-and-learnIssues related to the Code-and-Learn events and PRs submitted during the events.testIssues and PRs related to the tests.tlsIssues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@pauljlucas@gibfahn@bnoordhuis@jasnell@cjihrig@MylesBorins@addaleax@imyller@nodejs-github-bot