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: cleanup IIFE tests #7694

Merged
merged 1 commit into from
Jul 15, 2016
Merged

test: cleanup IIFE tests #7694

merged 1 commit into from
Jul 15, 2016

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jul 13, 2016

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

test

Description of change

A number of test files use IIFEs to separate distinct tests from each other in the same file. The project has been moving toward using block scopes and let/const in favor of IIFEs. This commit moves IIFE tests to block scopes. Some additional cleanup of these tests such as the use of assert.strictEqual() and common.mustCall() is also included.

R= @nodejs/testing

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 13, 2016
@santigimeno
Copy link
Member

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 13, 2016

@santigimeno CI was green.

@santigimeno
Copy link
Member

LGTM

A number of test files use IIFEs to separate distinct tests from
each other in the same file. The project has been moving toward
using block scopes and let/const in favor of IIFEs. This commit
moves IIFE tests to block scopes. Some additional cleanup such
as use of strictEqual() and common.mustCall() is also included.

PR-URL: nodejs#7694
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@cjihrig cjihrig merged commit 4a40832 into nodejs:master Jul 15, 2016
@cjihrig cjihrig deleted the iife branch July 15, 2016 02:13
evanlucas pushed a commit that referenced this pull request Jul 18, 2016
A number of test files use IIFEs to separate distinct tests from
each other in the same file. The project has been moving toward
using block scopes and let/const in favor of IIFEs. This commit
moves IIFE tests to block scopes. Some additional cleanup such
as use of strictEqual() and common.mustCall() is also included.

PR-URL: #7694
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
evanlucas pushed a commit that referenced this pull request Jul 20, 2016
A number of test files use IIFEs to separate distinct tests from
each other in the same file. The project has been moving toward
using block scopes and let/const in favor of IIFEs. This commit
moves IIFE tests to block scopes. Some additional cleanup such
as use of strictEqual() and common.mustCall() is also included.

PR-URL: #7694
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@MylesBorins
Copy link
Contributor

@cjihrig this is not landing cleanly on v4.x. Please feel free to manually backport

@bnoordhuis
Copy link
Member

Caveat emptor: this pull request introduces bugs in at least test/parallel/test-crypto-authenticated.js - the removal of the IIFE make the return statements stop the test instead of just skipping the subtest. See the discussion in #9032.

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 12, 2016

I'm reviewing all the tests in this PR and will submit a follow up that fixes any such bugs.

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 12, 2016

I see that @bnoordhuis covered test/parallel/test-crypto-authenticated.js in #9032. I've gone through all the tests in this PR, and that seems to be the only one that needs to be updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants