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.

Sign up for GitHub

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

Jump to bottom

[7.x] Unmute more memory-related tests after the fix in #68542 #68741

Merged
merged 1 commit into from Feb 19, 2021
Merged

[7.x] Unmute more memory-related tests after the fix in #68542 #68741

merged 1 commit into from Feb 19, 2021

Conversation

danhermann
Copy link
Contributor

Relates to #68542

@danhermann danhermann added >test Issues or PRs that are addressing/adding tests v7.12.0 v7.11.1 labels Feb 9, 2021
@droberts195
Copy link
Contributor

I think there was a problem with the workaround, so these tests will probably fail if unmuted. The Java security manager blocks reading /proc/meminfo - see for example https://gradle-enterprise.elastic.co/s/6qtcnglfzfdvg/tests/:server:test/org.elasticsearch.monitor.os.OsProbeTests/testGetTotalMemoryOnDebian8#1

@danhermann
Copy link
Contributor Author

I think there was a problem with the workaround, so these tests will probably fail if unmuted. The Java security manager blocks reading /proc/meminfo - see for example https://gradle-enterprise.elastic.co/s/6qtcnglfzfdvg/tests/:server:test/org.elasticsearch.monitor.os.OsProbeTests/testGetTotalMemoryOnDebian8#1

Ah, thanks for pointing that out, @droberts195.

@danhermann
Copy link
Contributor Author

@droberts195, I opened #68742 to address the security manager issue.

@danhermann
Copy link
Contributor Author

#68742 has been merged and the previously failing test on the debian8 platform passed, so I think this PR is clear to be reviewed.

probakowski
probakowski approved these changes Feb 19, 2021
View reviewed changes
Copy link
Contributor

@probakowski probakowski left a comment

Choose a reason for hiding this comment

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

LGTM, one minor nit

server/src/test/java/org/elasticsearch/monitor/os/OsProbeTests.java
@@ -8,15 +8,8 @@

package org.elasticsearch.monitor.os;

import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.anyOf;
Copy link
Contributor

Choose a reason for hiding this comment

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

These import changes are not needed for this change, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The commit that muted these tests (09f823b) moved the import statements out of the standard ordering that we use, so I just restored the original standard ordering as part of the un-mute.

@danhermann danhermann merged commit b311906 into elastic:7.x Feb 19, 2021
@danhermann danhermann deleted the unmute_more_memory_tests branch February 19, 2021 14:07
@danhermann danhermann removed the request for review from gwbrown February 19, 2021 14:11
This was referenced Feb 19, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@probakowski probakowski probakowski approved these changes

Assignees
No one assigned
Labels
>test Issues or PRs that are addressing/adding tests v7.11.2 v7.12.1 v7.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@danhermann @droberts195 @probakowski @williamrandolph @pugnascotia