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

test: add ThreadSchedTests #47

Merged
merged 2 commits into from
Oct 26, 2023
Merged

test: add ThreadSchedTests #47

merged 2 commits into from
Oct 26, 2023

Conversation

kartva
Copy link
Contributor

@kartva kartva commented Oct 4, 2023 •
edited
Loading

Is your feature request related to a problem? Please provide an existing Issue # , or describe.
Add tests for os/ThreadSched.java.

Check List

  • New functionality includes testing.
    • All tests pass
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

➜  ~/opensearch/performance-analyzer-commons ./gradlew test --tests ThreadSchedTests                                   

> Task :test

org.opensearch.performanceanalyzer.commons.os.ThreadSchedTests > testMetrics STARTED

org.opensearch.performanceanalyzer.commons.os.ThreadSchedTests > testMetrics PASSED

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/7.6.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 8s

@kartva kartva marked this pull request as draft October 4, 2023 14:13
@kartva
test: add ThreadSchedTests
5c7e8be 
verifies parsed procfiles are correctly read into TidKVMap
adds VisibleForTesting methods to ThreadSched

Signed-off-by: Kartavya Vashishtha <[email protected]>
@kartva
add testing for generated metrics
7cb3cae 
i.e. added testing for calculateSchedLatency

Signed-off-by: Kartavya Vashishtha <[email protected]>
@kartva kartva marked this pull request as ready for review October 5, 2023 00:51
ansjcy
ansjcy reviewed Oct 10, 2023
View reviewed changes
src/main/java/org/opensearch/performanceanalyzer/commons/os/ThreadSched.java
@@ -37,6 +39,27 @@ public static class SchedMetrics {
this.contextSwitchRate = contextSwitchRate;
}

@Override
public int hashCode() {
Copy link
Member

Choose a reason for hiding this comment

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

Why are those functions needed? Also if these functions will only be used in the unit tests, let's try adding some similar util functions in the test itself instead of adding new functions for the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These functions are used to implement equality, which is used to verify that the setSchedMetric is called with a ThreadSched.SchedMetrics that has the correct values.

i.e.

        verify(linuxSchedMetricsGenerator)
                .setSchedMetric("3", new ThreadSched.SchedMetrics(0.002, 0.002, 100.0));

at https://github.com/DesmondWillowbrook/performance-analyzer-commons/blob/threadsched-test/src/test/java/org/opensearch/performanceanalyzer/commons/os/ThreadSchedTests.java#L123-L124

Would you prefer something that uses the Reflection API instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ansjcy Does that explain why I implemented equality?

ansjcy
ansjcy reviewed Oct 10, 2023
View reviewed changes
src/main/java/org/opensearch/performanceanalyzer/commons/os/ThreadSched.java
return schedLatencyMap;
}

@VisibleForTesting
public Map<String, Map<String, Object>> getTidKVMap() {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can keep this one to verify the TidKVMap.

ansjcy
ansjcy reviewed Oct 10, 2023
View reviewed changes
src/test/java/org/opensearch/performanceanalyzer/commons/os/ThreadSchedTests.java

@RunWith(PowerMockRunner.class)
@PowerMockIgnore("javax.management.*")
@SuppressStaticInitializationFor({"org.opensearch.performanceanalyzer.commons.os.OSGlobals"})
Copy link
Member

Choose a reason for hiding this comment

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

Nice! it make perfect sense to avoid initializing the static classes imported from this class.

@ansjcy
Copy link
Member

ansjcy commented Oct 10, 2023

Let's add the output of the unit test in the PR description as well.

@kartva
Copy link
Contributor Author

kartva commented Oct 11, 2023 •
edited
Loading

@ansjcy

Let's add the output of the unit test in the PR description as well.

Running ./gradlew test --tests ThreadSchedTests gives me:

org.opensearch.performanceanalyzer.commons.os.ThreadSchedTests > testMetrics STARTED

org.opensearch.performanceanalyzer.commons.os.ThreadSchedTests > testMetrics PASSED

Is this what you meant by that? Or is it pasting the assertions that the unit test makes about the class being tested?

@kartva
Copy link
Contributor Author

kartva commented Oct 19, 2023

Alternatives to implementing equals on ThreadSched.SchedMetrics:

  • Reflection
  • Comparing the objects based on their toString representation
  • Writing getters for all their fields and comparing them one-by-one.

Out of which implementing equality seemed the simplest.

Turns out the test class does not meet the requirements to access a private member of ThreadSched.SchedMetrics, so using reflection seems infeasible.

@ansjcy ansjcy merged commit 7d2e1dd into opensearch-project:main Oct 26, 2023
4 checks passed
@kartva kartva mentioned this pull request Nov 8, 2023
Merged
3 tasks
@kartva kartva mentioned this pull request Nov 22, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ansjcy ansjcy ansjcy approved these changes

@sbcd90 sbcd90 sbcd90 approved these changes

@praveensameneni praveensameneni Awaiting requested review from praveensameneni praveensameneni is a code owner

@khushbr khushbr Awaiting requested review from khushbr khushbr is a code owner

@goyamegh goyamegh Awaiting requested review from goyamegh goyamegh is a code owner

@getsaurabh02 getsaurabh02 Awaiting requested review from getsaurabh02 getsaurabh02 is a code owner

@sgup432 sgup432 Awaiting requested review from sgup432 sgup432 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kartva @ansjcy @sbcd90