rjz / supertest-session

Persistent sessions for supertest
Other
88 stars 21 forks source link

supertest-session is causing mocha test suite to hang #28

Closed zeke closed 6 years ago

zeke commented 6 years ago

đź‘‹ @rjz thanks for creating this module. It looks like just what we need for testing the electron website.

I have the following test:

test('language query param for one-off viewing in other languages', async () => {
  const frenchContent = 'fenĂŞtres'
  const sesh = session(app)

  let res = await sesh.get('/docs/api/browser-window?lang=fr-FR')
  let $ = cheerio.load(res.text)
  $('blockquote').text().should.include(frenchContent)

  // verify that the query param does not persist as a cookie
  res = await sesh.get('/docs/api/browser-window')
  $ = cheerio.load(res.text)
  $('blockquote').text().should.not.include(frenchContent)
})

The test itself passes, but it causes mocha to hang forever after the suite has run. If I disable this single test, the problem goes away. I thought maybe this was an issue with async/await. I change the code to use Promises, then, and done, but it still hangs.

Any ideas?

Failing PR: https://github.com/electron/electronjs.org/pull/1034

rjz commented 6 years ago

Hey @zeke, and thanks for reporting!

This looks like an issue with the way the cookie jar's URL is initialized. I've pushed up a heavy-handed fix in https://github.com/rjz/supertest-session/pull/29 (9dc83a1) that should get the test suite back on track. When you have a moment, could you confirm that it works for you?

zeke commented 6 years ago

Thanks for the quick fix, @rjz. I installed rjz/supertest-session#9dc83a1a33b8c5d0aa134ea0032a870526ef0368 and it's working like a charm. đź‘Ť

rjz commented 6 years ago

Beauty! I'll clean that commit up and cut a new release shortly.

rjz commented 6 years ago

Published as supertest-session@3.1.1.

Thanks again for catching this, @zeke, and please be in touch if anything else comes up!

XVincentX commented 6 years ago

@rjz It seems like this change broke some expectations.

app, in my case, is an Object and not a Function. I already have a full working server, but I still need to extract the url from the app object itself.

rjz commented 6 years ago

I legitimately flipped a coin over whether the fix in #29 constituted a "patch" version or not. I lost.

Sorry about that, @XVincentX—will fix and update ASAP!

XVincentX commented 6 years ago

It is actually really weird how I come up to this bug.

I have a test suite for an open source project running on node 6,8 and 9.

Suddenly node6 build broke and I started to investigate, and it was pointing to supertest-session; I updated your library (since I noticed an update) and here I am now.

I still do not get what made the node6 build break suddenly 🤔

rjz commented 6 years ago

I was able to reproduce the issue and pushed up a quick fix (https://github.com/rjz/supertest-session/commit/3ae4930854329eee3ce75e4a2edfdbf880793d44), but it's weird that it's version dependent. I've added 6,8, and 9 to the travis build (#31)—hopefully CI will catch this in the future!

Sorry again for the trouble, and big thanks for homing in on the issue.