Skip to content

test/nginx: extract request() and add testing#1955

Merged
alxndrsn merged 4 commits into
getodk:nextfrom
alxndrsn:test-nginx-request-test
Jun 7, 2026
Merged

test/nginx: extract request() and add testing#1955
alxndrsn merged 4 commits into
getodk:nextfrom
alxndrsn:test-nginx-request-test

Conversation

@alxndrsn
Copy link
Copy Markdown
Contributor

@alxndrsn alxndrsn commented Jun 6, 2026

Added as a precursor to #1949.

What has been done to verify that this works as intended?

This does not change functional code, just adds a test for request() and extracts that function to its own file.

These tests can be seen to fail by substituting fetch() for request():

--- a/test/nginx/src/mocha/request.spec.js
+++ b/test/nginx/src/mocha/request.spec.js
@@ -4,9 +4,10 @@ const {
   assert,
 } = require('../lib');
 
-const request = require('./request');
+//const request = require('./request');
+const request = fetch;
  request()
    1) should not follow redirects
    2) should allow setting Host header


  0 passing (53ms)
  2 failing

  1) request()
       should not follow redirects:
     TypeError: fetch failed
  
     Caused by: Error: getaddrinfo ENOTFOUND example.test
      at GetAddrInfoReqWrap.onlookupall [as oncomplete] (node:dns:122:26)

  2) request()
       should allow setting Host header:

      AssertionError: expected '127.0.0.1:36363' to equal 'not-a-host'
      + expected - actual

      -127.0.0.1:36363
      +not-a-host
      
      at Context.<anonymous> (nginx/src/mocha/request.spec.js:68:12)
      at process.processTicksAndRejections (node:internal/process/task_queues:104:5)

Why is this the best possible solution? Were any other approaches considered?

request() is quite complicated, and deserves tests before future changes.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

No effect - just test code.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • branched off and targeted the next branch OR only changed documentation/infrastructure (master is stable and used in production)
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

@alxndrsn alxndrsn requested a review from matthew-white June 6, 2026 04:28
@alxndrsn alxndrsn marked this pull request as ready for review June 6, 2026 04:31
it('should allow setting Host header', async () => {
// given
const headers = {
'host': 'not-a-host', // FIXME also test with other cases, e.g. "Host" or "HOST"
Copy link
Copy Markdown
Contributor Author

@alxndrsn alxndrsn Jun 6, 2026

Choose a reason for hiding this comment

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

Reviewer: this is a bug in request(), but I didn't want to change any functionality in this PR.

@alxndrsn alxndrsn merged commit 0c07aaa into getodk:next Jun 7, 2026
7 checks passed
@alxndrsn alxndrsn deleted the test-nginx-request-test branch June 7, 2026 06:07
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.

2 participants