opensearch-project / ml-commons

ml-commons provides a set of common machine learning algorithms, e.g. k-means, or linear regression, to help developers build ML related features within OpenSearch.
Apache License 2.0
98 stars 136 forks source link

[CI] Inconsistent test configuration between modules #3119

Open dbwiddis opened 1 month ago

dbwiddis commented 1 month ago

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:

  1. 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).
  2. 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.
  3. 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?

  1. Align on a standard convention across the entire plugin
  2. Remove the incorrect ForbiddenAPI check saying the annotation isn't required
  3. Configure other modules to give useful info like plugin module currently does
  4. 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.
  5. 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).

dbwiddis commented 2 weeks ago

~Also, spotless doesn't run on common.~

Apparently it's been fixed past the version backported to the feature/multi_tenancy branch.