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

Use only fake domain in our tests #2197

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Nov 14, 2024

Summary

The ReactiveRestClientProxyIT test really talked to http://example.com, now no external communication to example.com is run, however we assert that correct host was proxied by passing $host via Nginx back to the test.

Please select the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • Dependency update
  • Refactoring
  • Backport
  • New scenario (non-breaking change which adds functionality)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@michalvavrik
Copy link
Member Author

run tests

@michalvavrik
Copy link
Member Author

run tests

@michalvavrik
Copy link
Member Author

There doesn't seem to be a sense to rerun OCP now, I'll try it tomorrow as the cluster is in a bad shape.

@michalvavrik
Copy link
Member Author

OCP test failures are not related, but I need to re-think that proxy stuff I guess. Not sure.

@michalvavrik
Copy link
Member Author

I thought about it and what we need to test is that proxy options are set correctly, not the redirect to external domain.

@michalvavrik
Copy link
Member Author

run tests

@michalvavrik
Copy link
Member Author

@mocenas OCP native run didn't fail, I canceled it manually because it was started when OCP cluster was down and now it is waiting in a queue. I won't merge this till it is green, so please review when you get a time. Thanks

@mocenas
Copy link
Contributor

mocenas commented Nov 15, 2024

I thought about it and what we need to test is that proxy options are set correctly, not the redirect to external domain.

I thought about it too. It "would be nice" if we could do an E2E test to verify that communication though proxy works. But as you states, we want to test connection and options-passing to the proxy, not proxy's ability to forward it outside.

Copy link
Contributor

@mocenas mocenas left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Don't connect to external domains in tests
2 participants