-
Notifications
You must be signed in to change notification settings - Fork 196
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
Reset environment proxy config for some cache and range tests. #175
Conversation
df118e6
to
eb01155
Compare
From the failing Travis log:
I'll just re-commit and hope the next test run has better luck with the random numbers. This might be more interesting:
|
eb01155
to
e564b74
Compare
Environment proxy config? what? |
The environment variables |
Interesting. Is it possible to set these while running tests? Say, |
The express test is probably easy to fix by doing a search in the test file for |
I think a good solution would be to refactor a lot of these patterns into a test helper module (maybe
That pre-configured instance of request could bypass env proxy settings:
I'm not eager to refactor all those tests myself, so anyone interested please jump in if @jfhbrook approves of the idea. Since I can't find a mention of OS-assigned ports in the var srv = require('http').createServer();
srv.listen(0, 'localhost', function serverReady(err) {
var addr = srv.address(),
baseUrl = 'http://' + addr.address + ':' + addr.port + '/';
console.log(baseUrl); // http://127.0.0.1:43482/
srv.close();
}); |
Now back to your questions:
It is. The post above shows a more elegant way than using $ export DUMMY=hi; echo a $DUMMY; nodejs -e '
> console.log(1, process.env.DUMMY);
> delete process.env.DUMMY;
> console.log(2, process.env.DUMMY);
> '; echo b $DUMMY
a hi
1 'hi'
2 undefined
b hi
That should work as well. I used
… but that would kill any chance to set a custom proxy.
Then tests should at least warn that proxy settings are in effect. If the tests default to using system proxy settings, they should be liberal in what they accept from that proxy. Giving some thousand lines of terminal output complaining about transport headers isn't what I had expected from "running the tests before I start tinkering, just to be sure." ;-) If the proxy settings should be test-specific, the request approach seems fit. For someone who wants to test a special proxy, we should allow them to specify one explicitly for the ecstatic tests, to have a conscious decision about the interception and the potential for test failures. We could use an environment variable |
3974b20
to
0e306dd
Compare
@jekrb did some tests for setting the port via an environment variable in About the 0.0.0.0 pattern, I think it's a unix misconception about localhost. Unfortunately |
In case there should ever be a security flaw in ecstatic or the file system it uses for its tests, there may be effective differences between listening on "localhost" or "any address". However, for tests of external listening, here's my example code adapted: var srv = require('http').createServer();
srv.listen(0, '', function serverReady(err) {
var addr = srv.address(), host = String(addr.address), baseUrl;
if (host.match(/^[0\.:]+$/)) { host = 'localhost'; }
// ^-- e.g. "0.0.0.0", "::", "::0"
baseUrl = 'http://' + host + ':' + addr.port + '/';
console.log(baseUrl); // http://localhost:48592/
srv.close();
}); |
Yeah, that seems reasonable. I'll turn this into an issue.
This would seem like the way to go, except I don't think I can support both windows and nix with that. |
I think the best approach here is to document the issue in CONTRIBUTING.md. I'm not sure how to reproduce the behavior you were seeing, so I'll leave that up to you. I'm going to close this PR for now, not because HTTP_PROXY isn't an issue, but because I don't think this is the right PR to merge. I'll make a separate issue for this, and link in this thread. |
The trick that made those tests pass on my test computer.