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

test: cache lazy properties, fix style nits #1196

Merged
merged 1 commit into from
Mar 19, 2015

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Mar 18, 2015

(addressing the concerns I raised too late in #1167)

inFreeBSDJail involves an execSync() and is used by localhost_ipv4 so will be unnecessarily expensive, so cache both values and reuse rather than re-evaluate each time.

Renamed localhost_ipv4 to localhostIPv4 for style consistency.

/R=@jbergstroem
/R=@bnoordhuis

@rvagg
Copy link
Member Author

rvagg commented Mar 18, 2015

'as LOCALHOST or expect some tests to fail.');
}
} else {
localhostIPv4 = '127.0.0.1';
Copy link
Member

Choose a reason for hiding this comment

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

This will never be reached for FreeBSD jails that hasn't set LOCALHOST. I think falling back to '127.0.0.1' is the right thing here.

@jbergstroem
Copy link
Member

Sorry for being too quick on the gun in #1167. Minor nit otherwise LGTM.

@rvagg rvagg force-pushed the cleanup-freebsd-test-exports branch from f3f6f6d to 11042de Compare March 19, 2015 00:29
@rvagg
Copy link
Member Author

rvagg commented Mar 19, 2015

@jbergstroem ptal, I've changed the logic around the default fallback

@jbergstroem
Copy link
Member

:shipit: LGTM – thanks again for improving my version.

(am I the only one that's annoyed by the alignment of the shipit icon?)

inFreeBSDJail involves an execSync() and is used by localhost_ipv4 so
will be unnecessarily expensive, so cache both values and reuse
rather than re-evaluate each time.

Renamed localhost_ipv4 to localhostIPv4 for style consistency.

PR-URL: nodejs#1196
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@rvagg rvagg force-pushed the cleanup-freebsd-test-exports branch from 11042de to f600111 Compare March 19, 2015 05:28
@rvagg rvagg merged commit f600111 into nodejs:v1.x Mar 19, 2015
@rvagg rvagg deleted the cleanup-freebsd-test-exports branch March 19, 2015 05:28
@rvagg
Copy link
Member Author

rvagg commented Mar 19, 2015

landed @ f600111

@rvagg rvagg mentioned this pull request Mar 19, 2015
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