NIFI-16265 - Extract reusable System Test framework from nifi-system-test-suite

[markap14]

Summary

• Adds a new nifi-system-test-framework module containing NiFiSystemIT and its supporting infrastructure (NiFiInstance, InstanceConfiguration, SpawnedStandaloneNiFiInstanceFactory, SpawnedClusterNiFiInstanceFactory, AggregateNiFiInstance, NiFiInstanceCache, NiFiClientUtil, NiFiSystemKeyStoreProvider, TroubleshootingTestWatcher, etc.), moved to src/main/java so the artifact is a normal, publishable dependency rather than test-scoped code trapped inside nifi-system-test-suite.
• Third-party NAR projects can now depend on nifi-system-test-framework (as a test-scope dependency, the same way nifi-mock is depended on for unit tests) to write their own System Tests that spawn a real NiFi instance and drive it via the REST client, without needing a full NiFi source checkout.
• nifi-system-test-suite now depends on the new module instead of defining these classes itself. It retains all of its own *IT test classes, test resources, and the NiFi-specific AbstractNarSwapMigrationIT base class (which is tied to internal test-only NARs and isn't generic).
• No behavior changes: package names are unchanged (org.apache.nifi.tests.system), so none of the ~500 existing *IT test classes required edits.

Test plan

• mvn -pl nifi-system-tests/nifi-system-test-framework -am install -DskipTests — new module compiles and installs.
• mvn -pl nifi-system-tests/nifi-system-test-suite -am install -DskipTests — suite compiles against the new dependency.
• mvn -pl nifi-system-tests/nifi-system-test-suite verify -Pintegration-tests,skip-unit-tests -Dit.test=ClassloaderIsolationKeyIT — ran a real System Test end-to-end (spawns a NiFi instance) using NiFiSystemIT from the extracted module: Tests run: 2, Failures: 0, Errors: 0.

[exceptionfactory]

Thanks for putting together this refactored packaging for system tests @markap14.

There is definitely value in support external use of these tests, but on initial read, I'm concerned about the implied visibility of some of these classes.

I have not reviewed the classes in detail to consider the full implications, but do you think that some of the can be package-private, or do all of them need to be public? Classes like the System KeyStore Provider seem like that might not need public visibility.

[markap14]

That's fair @exceptionfactory. My intent was to avoid changing anything and instead move everything over exactly as-is. But you're right, moving from test to publicly available it is probably fair to reduce visibility on some of those.

[markap14]

Good catch, thanks @exceptionfactory. I went through the moved classes and checked (via grep across all ~500 existing `*IT` test classes, which live in subpackages of `org.apache.nifi.tests.system`) which ones are actually referenced from outside the framework's own package.

`NiFiInstanceProvider`, `AggregateNiFiInstance`, `NiFiInstanceCache`, `NiFiSystemKeyStoreProvider`, and `TroubleshootingTestWatcher` (including the one you called out) have no external consumers - they're only used internally by `NiFiSystemIT` and its factory implementations, so I made them package-private in the latest commit.

`NiFiSystemIT`, `NiFiInstance`, `NiFiInstanceFactory`, `InstanceConfiguration`, `SpawnedStandaloneNiFiInstanceFactory`, `SpawnedClusterNiFiInstanceFactory`, `NiFiClientUtil`, and `ExceptionalBooleanSupplier` do need to stay public - they're directly extended/constructed/imported from test classes in other packages today.

Rebuilt and reran the sample IT test to confirm nothing broke.

[exceptionfactory]

Thanks @markap14, adjusting the visibility on those classes helps focus the supported surface.

My remaining concerns relate to the large number of dependencies that this module pulls in. A number of these are expected based on framework requirements, but it also makes for a large dependency tree that raises some additional questions in terms of maintainability as a more public-facing library. Although not on the same level as something like the nifi-framework-api, making the test framework available as "framework extension" highlights some need to scope down dependencies as much as possible.

With that in mind, what options are available for restructuring the loading process to more clearly distinguish dependencies? Although it would require one additional module, it makes me wonder whether there should be a "system-test-framework-api" and a "system-test-framework" to provide stronger boundaries. Things like Jersey, the Framework Cluster Protocol, and the Python Framework API are among dependencies that seem like they should not bleed into a module intended for extension. Open to options here on possible approaches.

[markap14]

@exceptionfactory I'm not sure that I would call this "a module intended for extension." This is a module intended to start up 1 nifi instance or one full cluster of nifi instances in a light-weight manner purely with the goal of verifying behavior against a legit, running instance for automated tests. I don't think we can remove things like Jersey or the Framework Cluster Protocol because doing so would fail to run NiFi as it's run in production. Please correct me if I'm misunderstanding something.

[exceptionfactory]

By module for extension, I was referring specifically to NiFiSystemIT as a base test class that others would extend. The other remaining public classes also imply the need for reuse. That's based on the usage in existing system tests. Is there a different way that system test builders would use this module?

At the root, I'm concerned about the lack of encapsulation in the current approach. The design has worked well when everything was co-located in the system test suite, but it seems like just moving out these classes doesn't provide the kind of approach needed for test interfaces versus test implementation.

Taking a different perspective, it also raises a question as to whether a Testcontainer approach might be better, as that would encapsulate the entire application, without these types of module contract concerns. I realize this is a significantly different path, but it seems worth considering as an alternative.

[markap14]

Ahh I see what you're saying now @exceptionfactory. I don't think Testcontainers is really the way we want to go here because that would require that the full docker image be built so it could be usable. That would absolutely make sense for testing extensions such as Processors in a separate service (by uploading the nar, etc). But it wouldn't work for framework-level extensions.

That said, I reviewed the pom.xml again and I think I see the concern. With the latest commit we now have basically only 2 sets of dependencies:

• Those on the nifi root lib/ directory (these are necessary) to start the nifi instance
• The nifi-toolkit-client necessary for communicating with the nifi instance. This includes the jersey dependency. An argument can definitely be made that it's actually a bug in the toolkit-client code that it's not explicitly defined there, but it's necessary at runtime.

The pom that I had factored out did have a few extra dependencies that I was able to eliminate with some simple refactoring:

• nifi-per-process-group-logging (not actually needed in the 'framework' module, just the test that uses it)
• commons-lang3, nifi-xml-processing (not actually necessary for the 'framework' module either)
• nifi-framework-cluster-protocol (used by the framework module but it was just for access to enums to compare to DTO String values, etc. so I was able to do some very simple minor refactoring to eliminate the dependency).