Skip to content

test: improved http vs https handling #1768

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

Merged
merged 20 commits into from
Jun 18, 2025
Merged

test: improved http vs https handling #1768

merged 20 commits into from
Jun 18, 2025

Conversation

kirrg001
Copy link
Contributor

@kirrg001 kirrg001 commented Jun 12, 2025

Problems:

  • NODE_TLS_REJECT_UNAUTHORIZED can create a native console log span (the warning comes from Nodejs as console log on runtime). HTTPS without certification -> creates a node warning. This can create real problems if you only assert the length of spans to expect
  • As part of fixing a bug in test(aws-lambda): clarified many spans tests #1767, I wanted to fix a test, which suddenly showed me this console span and it was part of a span assertion lengths!
  • We switch to http default. There is no reason to run tests on https by default. As long as https is covered.
  • Improved naming of variables. It was really chaotic.

@kirrg001 kirrg001 force-pushed the fix-tlsgg branch 2 times, most recently from 132e8ec to 1d2e2ff Compare June 17, 2025 21:41
@kirrg001 kirrg001 changed the title test: run backend stub by default on http test: improved http vs https handling Jun 17, 2025
// we need to disable the TLS certificate check, otherwise the request will fail.
// Refer to the problem discussed in https://github.com/node-fetch/node-fetch/issues/1
if (this.backendUsesHttps) {
process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not set NODE_TLS_REJECT_UNAUTHORIZED globally for a whole test env.
Only if the backend / app runs in https and we use self signed certs.

DOWNSTREAM_DUMMY_URL: this.downstreamDummyUrl,
INSTANA_DISABLE_CA_CHECK: true,
INSTANA_DEV_SEND_UNENCRYPTED: this.opts.backendUsesHttps ? 'false' : 'true',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

HTTP is default for the test env and sending INSTANA_DEV_SEND_UNENCRYPTED will allow to use an http endpoint.

@kirrg001 kirrg001 marked this pull request as ready for review June 17, 2025 23:16
@kirrg001 kirrg001 requested a review from a team as a code owner June 17, 2025 23:16
@kirrg001 kirrg001 merged commit 36c5a85 into main Jun 18, 2025
1 check passed
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