Skip to content

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

Merged
danhermann merged 1 commit intoelastic:7.xfrom
danhermann:unmute_more_memory_tests
Feb 19, 2021
Merged

[7.x] Unmute more memory-related tests after the fix in #68542#68741
danhermann merged 1 commit intoelastic:7.xfrom
danhermann:unmute_more_memory_tests

Conversation

@danhermann
Copy link
Copy Markdown
Contributor

@danhermann danhermann commented Feb 9, 2021

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
Copy Markdown

droberts195 commented Feb 9, 2021

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
Copy Markdown
Contributor Author

danhermann commented Feb 9, 2021

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
Copy Markdown
Contributor Author

danhermann commented Feb 9, 2021

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

@danhermann
Copy link
Copy Markdown
Contributor Author

danhermann commented Feb 11, 2021

#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
Copy Markdown
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
package org.elasticsearch.monitor.os;

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

@probakowski probakowski Feb 19, 2021

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
Copy Markdown
Contributor Author

@danhermann danhermann Feb 19, 2021

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 AthenaEryma 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

1 more reviewer

@probakowski probakowski probakowski approved these changes

Reviewers whose approvals may not affect merge requirements

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.

5 participants

@danhermann @droberts195 @probakowski @williamrandolph @pugnascotia