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

Loosen constraints on the jvm heap size for tests in extra_jvm_options #8106

Merged

Conversation

@blorente
Copy link
Contributor

commented Jul 24, 2019

Problem

We set the default heap size to 1GB, and there was a test that trusted on that being a consistent number. However, the test doesn't need that to work. It's not testing the actual default heap size, but that we can modify the heap size if we pass an option.

This was causing travis failures: https://pastebin.com/knEMAWm7

Solution

Only output a number if the heap size is less than 100MB. We set it to be very small in the target that we are testing.

Result

Hopefully no failures.

@stuhood
Copy link
Member

left a comment

The "default" heap size without any options set is ill-defined: it's chosen by the JVM based on the host. So I don't think that checking a range like this necessarily accomplishes what we wanted.

If we want to test this property in particular you could maybe set it to a much more specific value like -Xmx123456789?

Going to leave this to others more familiar with topic.

@blorente

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

The "default" heap size without any options set is ill-defined: it's chosen by the JVM based on the host. So I don't think that checking a range like this necessarily accomplishes what we wanted.

So the range here is not important, we just want to check that we haven't overwritten the value set by the machine's default (which, I agree, is unknowable).

If we want to test this property in particular you could maybe set it to a much more specific value like -Xmx123456789?

This is a good point, I'm guessing this would go under --run-java-jvm-options. Will give it a try.

@blorente blorente requested a review from stuhood Jul 25, 2019

@blorente blorente merged commit 5967b2f into pantsbuild:master Jul 25, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.