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: unit test for proxy pagination handling #56

Open
wants to merge 19 commits into
base: v2
Choose a base branch
from

Conversation

barabo
Copy link

@barabo barabo commented Jul 23, 2021

There were a few issues with the pagination unit test for the proxy.

  • it started with a relative URL and then used a full url to follow the 'next' link...
  • but the test environment was not using the same hostname & port as what was in the 'next' link...
  • so the test was silently throwing away a 'connection refused error' and passing anyway.
  • the second half of the test incorrectly referenced the first 'res' result when it seemed to intend to reference the second 'res2' (which was unreferenced)
  • after running npm test - the mocha test framework process hung because the second get call failed (I believe, but still not 100% sure why). Running the tests with the npm test -- --exit flag was a workaround, but felt wrong.

After applying these changes npm test works without having to use the --exit flag, and the intended coverage is working again.

barabo and others added 2 commits July 23, 2021 00:23
There were a few issues with the pagination unit test for the proxy.
* it started with a relative URL and then used a full url to follow the 'next' link
* the test environment was not using the same hostname & port as what was in the 'next' link
* it was incorrectly referencing the first 'res' result when it seemed to intend to reference the second 'res2'
* after running npm test - the process hung because the second get call failed (I believe)

After applying these changes npm test works without having to use the --exit flag, and the intended coverage is working again.
@jmandel
Copy link
Member

jmandel commented Jul 24, 2021

There's a subtle underlying issue where mocha starts the server with a random ephemeral port, but the server believes/expects its port to be whatever's defined in config -- so during tests, searches return invalid Bundle.link.url values.

microsoft-healthcare-madison@965cbfe#diff-f1c404872f48ce2b9a4a9e18262e68a64bb43cbc934a8a4d891d098a1b3a3b62 may be
a better way to approach this, by starting the server in mocha hooks explicitly, ensuring that the actual port matches the configured port.

@vlad-ignatov
Copy link
Contributor

There are multiple issues here, but mostly:

  1. The res/res2 mismatch
  2. The random port problem

Your solution looks definitely cleaner, but also a little harder to read. I think I've fixed these in 92fec07. Please give it a try and feel free to reapply your changes on top of that if needed.

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.

3 participants