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: linting for buffer-free-callback test #3230

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 7, 2015

Test added in d1f2404 does not pass linting rule added in 3de353b.
Fixed in this commit. common module required in all tests except
those that intentionally leak variables.

Fixes: #3229

Test added in d1f2404 does not pass linting rule added in 3de353b.
Fixed in this commit. `common` module required in all tests except
those that intentionally leak variables.

Fixes: nodejs#3229
@Trott
Copy link
Member Author

Trott commented Oct 7, 2015

@Trott Trott added the test Issues and PRs related to the tests. label Oct 7, 2015
@Trott
Copy link
Member Author

Trott commented Oct 7, 2015

/cc @indutny This minor change to your test look good to you?

@Fishrock123
Copy link
Contributor

LGTM

@indutny
Copy link
Member

indutny commented Oct 7, 2015

@Trott I didn't see any errors when committing it. Are you sure that it is required for addons?

@Trott
Copy link
Member Author

Trott commented Oct 7, 2015

@indutny Yes, I'm postive.

$ git checkout  d1f2404 
Note: checking out 'd1f2404'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b new_branch_name

HEAD is now at d1f2404... buffer: FreeCallback should be tied to ArrayBuffer
$ make jslint
./node tools/eslint/bin/eslint.js src lib test tools/eslint-rules \
        --rulesdir tools/eslint-rules --reset --quiet

test/addons/buffer-free-callback/test.js
  1:0  error  Mandatory module "common" must be loaded  required-modules

✖ 1 problem (1 error, 0 warnings)

make: *** [jslint] Error 1
$

@indutny
Copy link
Member

indutny commented Oct 7, 2015

Ok, LGTM then.

@indutny
Copy link
Member

indutny commented Oct 7, 2015

Shouldn't it go to other addons too?

@Trott
Copy link
Member Author

Trott commented Oct 7, 2015

Yes, all the previously existing addon tests that didn't use common got it in c78091d which is the commit immediately prior to the linting rule.

@Trott
Copy link
Member Author

Trott commented Oct 7, 2015

Because this change is so small and CI will fail until it lands, I'm going to go ahead and land it right away.

Trott added a commit that referenced this pull request Oct 7, 2015
Test added in d1f2404 does not pass linting rule added in 3de353b.
Fixed in this commit. `common` module required in all tests except
those that intentionally leak variables.

PR-URL: #3230
Fixes: #3229
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Trott
Copy link
Member Author

Trott commented Oct 7, 2015

Landed in 6f63a4a

@Trott Trott closed this Oct 7, 2015
@jasnell jasnell mentioned this pull request Oct 8, 2015
29 tasks
Trott added a commit that referenced this pull request Oct 8, 2015
Test added in d1f2404 does not pass linting rule added in 3de353b.
Fixed in this commit. `common` module required in all tests except
those that intentionally leak variables.

PR-URL: #3230
Fixes: #3229
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Trott Trott deleted the fix-d1f24044 branch January 13, 2022 22:28
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.

4 participants