You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Tests run in the :opensearch-ml-plugin module behave differently than in :opensearch-ml-common.
Specific pain points:
The JUnit @Test annotation is required in the common and many other modules. However, it is not required anywhere in the plugin module since the build.gradle file has a test { include '**/*Tests.class' } configuration, which executes all methods in classes ending with Tests as tests. This is confusing, leading to inadvertent disabling of tests when refactoring them from the plugin module (where they worked) to the common module (where they didn't even run).
Forbidden APIs checks only seem to be configured in the plugin module and spam the build logs (only for the plugin module) saying Forbidden annotation use: org.junit.Test [defaultMessage Just name your test method testFooBar] 284 times. This is actually inaccurate since there are no method name checks at all, it should really say what to name your class. This is bad for two reasons: it's confusing and leads to leaving off @Test annotation outside the plugin module for anyone who's looked at a lot of these logs; and it's extremely spammy adding 284*2=568 useless lines of text to the log.
Failing tests in the plugin module give the useful REPRODUCE WITH: ./gradlew ':opensearch-ml-plugin:test' summary with logs and a stack trace. However, failing tests in the common module just tell me that a test method failed, giving its line number and no other details. I can force them to show locally with gradle --info and --stacktrace switches, but that doesn't help when trying to diagnose CI build logs.
What solution would you like?
Align on a standard convention across the entire plugin
Remove the incorrect ForbiddenAPI check saying the annotation isn't required
Configure other modules to give useful info like plugin module currently does
And while I'm asking about tests, PLEASE don't cancel other tests when CI fails on one operating system. Given how flaky tests currently are in this repo, it should not be assumed that a single failing test on one OS means all will fail. This can easily be done by adding fail-fast: false under the strategy: entry for the build matrix, at least until the gradle check consistently passes. See example.
And while I'm talking about tests, PLEASE consider having the jacoco test coverage upload occur before failing the build on insufficient coverage. It's presently an absurd situation where all your tests pass but then the build fails because you didn't hit the threshold but have zero information on exactly what lines aren't covered without jumping through a lot of hoops. Consider separating out unit tests (that produce the coverage report) from integ tests (which are where the flakiness is) as a solution here. Consider installing the delta-coverage plugin to let contributors check their diff coverage locally with ./gradlew test deltaCoverage.
What alternatives have you considered?
Making the requested changes locally when contributing to this project and then reverting them before submitting PRs.
Do you have any additional context?
OpenSearch test cases do not use the @Test annotation. It would be nice if we aligned on a project-wide standard, but failing that, we should at least have the same standard in all modules, or clearly document why they are different.
Personally I prefer not requiring the @Test annotation, which makes test methods "default on" and are less likely to be accidentally skipped (as I've done regularly, see #3118).
The text was updated successfully, but these errors were encountered:
Is your feature request related to a problem?
Tests run in the
:opensearch-ml-plugin
module behave differently than in:opensearch-ml-common
.Specific pain points:
@Test
annotation is required in thecommon
and many other modules. However, it is not required anywhere in theplugin
module since thebuild.gradle
file has atest { include '**/*Tests.class' }
configuration, which executes all methods in classes ending withTests
as tests. This is confusing, leading to inadvertent disabling of tests when refactoring them from the plugin module (where they worked) to the common module (where they didn't even run).plugin
module and spam the build logs (only for theplugin
module) sayingForbidden annotation use: org.junit.Test [defaultMessage Just name your test method testFooBar]
284 times. This is actually inaccurate since there are no method name checks at all, it should really say what to name your class. This is bad for two reasons: it's confusing and leads to leaving off@Test
annotation outside theplugin
module for anyone who's looked at a lot of these logs; and it's extremely spammy adding 284*2=568 useless lines of text to the log.plugin
module give the usefulREPRODUCE WITH: ./gradlew ':opensearch-ml-plugin:test'
summary with logs and a stack trace. However, failing tests in thecommon
module just tell me that a test method failed, giving its line number and no other details. I can force them to show locally with gradle--info
and--stacktrace
switches, but that doesn't help when trying to diagnose CI build logs.What solution would you like?
fail-fast: false
under thestrategy:
entry for the build matrix, at least until the gradle check consistently passes. See example../gradlew test deltaCoverage
.What alternatives have you considered?
Making the requested changes locally when contributing to this project and then reverting them before submitting PRs.
Do you have any additional context?
OpenSearch test cases do not use the
@Test
annotation. It would be nice if we aligned on a project-wide standard, but failing that, we should at least have the same standard in all modules, or clearly document why they are different.Personally I prefer not requiring the
@Test
annotation, which makes test methods "default on" and are less likely to be accidentally skipped (as I've done regularly, see #3118).The text was updated successfully, but these errors were encountered: