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

fix(tests): fix hang in ioredis test #1839

Merged
merged 1 commit into from
Oct 26, 2020
Merged

Conversation

trentm
Copy link
Member

@trentm trentm commented Oct 26, 2020

[email protected] includes a bug when using redis.quit() that can leave
a setInterval uncleared. See redis/ioredis#1215.

Fixes #1838

[email protected] includes a bug when using `redis.quit()` that can leave
a setInterval uncleared. See redis/ioredis#1215.

Fixes #1838
@trentm trentm requested a review from astorm October 26, 2020 22:38
@trentm
Copy link
Member Author

trentm commented Oct 26, 2020

Example of a fixed test run:

[15:37:59 trentm@purple:~/el/apm-agent-nodejs (trentm-fix-ioredis-test-hang)]
$ node --unhandled-rejections=strict test/instrumentation/modules/ioredis.js
TAP version 13
# not nested
ok 1 null
ok 2 should be strictly equal
ok 3 null
ok 4 should be strictly equal
ok 5 should be strictly equal
ok 6 null
ok 7 should be deeply equivalent
ok 8 should be strictly equal
ok 9 should be strictly equal
ok 10 should be strictly equal
ok 11 should be strictly equal
ok 12 should be strictly equal
ok 13 should be strictly equal
ok 14 should be strictly equal
ok 15 should be strictly equal
ok 16 should be strictly equal
ok 17 should be truthy
ok 18 should be strictly equal
ok 19 should be strictly equal
ok 20 should be strictly equal
ok 21 should be truthy
ok 22 should be strictly equal
ok 23 should be strictly equal
ok 24 should be strictly equal
ok 25 should be truthy
ok 26 should be strictly equal
ok 27 should be strictly equal
ok 28 should be strictly equal
ok 29 should be truthy
ok 30 should be strictly equal
ok 31 should be strictly equal
ok 32 should be strictly equal
ok 33 should be truthy
ok 34 should be strictly equal
ok 35 should be strictly equal
ok 36 should be strictly equal
ok 37 should be truthy
ok 38 should be strictly equal
ok 39 should be strictly equal
ok 40 should be strictly equal
ok 41 should be truthy
ok 42 should be strictly equal
ok 43 should be strictly equal
ok 44 should be strictly equal
ok 45 should be truthy
# nested
ok 46 null
ok 47 should be strictly equal
ok 48 null
ok 49 should be strictly equal
ok 50 should be strictly equal
ok 51 null
ok 52 should be deeply equivalent
ok 53 should be strictly equal
ok 54 should be strictly equal
ok 55 should be strictly equal
ok 56 should be strictly equal
ok 57 should be strictly equal
ok 58 should be strictly equal
ok 59 should be strictly equal
ok 60 should be strictly equal
ok 61 should be strictly equal
ok 62 should be truthy
ok 63 should be strictly equal
ok 64 should be strictly equal
ok 65 should be strictly equal
ok 66 should be truthy
ok 67 should be strictly equal
ok 68 should be strictly equal
ok 69 should be strictly equal
ok 70 should be truthy
ok 71 should be strictly equal
ok 72 should be strictly equal
ok 73 should be strictly equal
ok 74 should be truthy
ok 75 should be strictly equal
ok 76 should be strictly equal
ok 77 should be strictly equal
ok 78 should be truthy
ok 79 should be strictly equal
ok 80 should be strictly equal
ok 81 should be strictly equal
ok 82 should be truthy
ok 83 should be strictly equal
ok 84 should be strictly equal
ok 85 should be strictly equal
ok 86 should be truthy
ok 87 should be strictly equal
ok 88 should be strictly equal
ok 89 should be strictly equal
ok 90 should be truthy
# rejections_handled
ok 91 should be truthy
ok 92 should be falsy

1..92
# tests 92
# pass  92

# ok

[15:38:56 trentm@purple:~/el/apm-agent-nodejs (trentm-fix-ioredis-test-hang)]
$

@trentm trentm self-assigned this Oct 26, 2020
@apmmachine
Copy link
Contributor

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #1839 opened]

  • Start Time: 2020-10-26T22:38:07.514+0000

  • Duration: 14 min 8 sec

Test stats 🧪

Test Results
Failed 0
Passed 16398
Skipped 0
Total 16398

Copy link
Contributor

@astorm astorm left a comment

Choose a reason for hiding this comment

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

Looks good from here, approving pending CI passing.

It looks like disconnect is "the right" way to shut things with all the ioredis internal bookkeeping. I don't see a quit method defined on that class so I assume it's issuing a quit directly to Redis via some sort of JS or TypeScript property magic.

@trentm trentm merged commit b016673 into master Oct 26, 2020
@trentm trentm deleted the trentm-fix-ioredis-test-hang branch October 26, 2020 23:01
@trentm
Copy link
Member Author

trentm commented Oct 26, 2020

I don't see a quit method defined on that class so I assume it's issuing a quit directly to Redis via some sort of JS or TypeScript property magic.

Yup, I did the same hunt. It results in calling https://redis.io/commands/quit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ioredis test hangs with [email protected]
3 participants