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.

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

4.x: Global instance handling change (and relevant changes to testing) #9193

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

tomas-langer
Copy link
Member

@tomas-langer tomas-langer commented Aug 22, 2024

Resolves #9186

  • using ContextValue that uses Context as the "backing" store
  • modified Metrics, ServiceRegistry, Config, and MapperManager to use ContextValue
  • ContextValue either uses the test context, or global context; this is achieved through a context registered under a known qualifier, to make sure it all works even when running in child context

Introduction of new test annotations

  • configuration setup using annotations for Helidon SE (through service registry)

  • static configuration update (if value was not yet read, and is not provided by existing config sources)

  • introduction of new JUnit5 annotation + extension (resets global instances)

  • Fix security's ClassToInstanceStore (discovered an issue while copying some ideas to GlobalInstances)

  • Add support for ExistingInstanceDescriptor to service registry (for testing purposes - bind an instance without a service descriptor)

  • Testing JUnit extensions now extend the new extension, so all tests inherit the "nice" behavior of global instances SE testing now supports ServiceRegistry (and is ready for future setup using Helidon Inject)

@tomas-langer tomas-langer added testing 4.x Version 4.x labels Aug 22, 2024
@tomas-langer tomas-langer added this to the 4.2.0 milestone Aug 22, 2024
@tomas-langer tomas-langer self-assigned this Aug 22, 2024
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Aug 22, 2024
…, yet support testing

Refactored existing global instances to use it
EnumMapperProvider as a proper service
Support for existing instance descriptor, to be able to register custom instances during tests (even for types that do not have a service descriptor) + fixed appropriate code in registry, which used Set instead of List
Fixed typo in registry module info
… will work nicely once Helidon Service Inject is introduced
- sets up registry with appropriate configuration (through annotations, test source, or static methods)
- resets global instances when test class is done
This also binds the current context to the test class that is executed, making parallel testing a possibility.
@tomas-langer tomas-langer marked this pull request as ready for review September 3, 2024 07:25
…entation.

Also removed the no-op, as it is no longer needed
Using a more specific classifier.
danielkec
danielkec previously approved these changes Sep 11, 2024

/**
* Set a configuration key and value.
* This method CANNOT override an existing key, as such keys are already in the config snapshot.
Copy link
Contributor

Choose a reason for hiding this comment

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

FAQ topic candidate

Copy link
Contributor

@romain-grecourt romain-grecourt Sep 11, 2024

Choose a reason for hiding this comment

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

Do we even need this method ?
This looks like a hack that we aren't using, at least in this pull request.

One should be able to use this instead:

@TestConfig.Source
static ConfigSource config() {
    var config = TestConfigSource.create();
    onPseudoEvent(() -> config.set("foo", "bar"));
    return config;
}

*
* @return instance of a test config source.
*/
public static TestConfigSource create() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should provide another variant that takes a initial map.
E.g.

TestConfigSource.create(Map.of("foo", "bar"));


/**
* Set a configuration key and value.
* This method CANNOT override an existing key, as such keys are already in the config snapshot.
Copy link
Contributor

@romain-grecourt romain-grecourt Sep 11, 2024

Choose a reason for hiding this comment

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

Do we even need this method ?
This looks like a hack that we aren't using, at least in this pull request.

One should be able to use this instead:

@TestConfig.Source
static ConfigSource config() {
    var config = TestConfigSource.create();
    onPseudoEvent(() -> config.set("foo", "bar"));
    return config;
}

*/
public final class ContextSingleton<T> {
/**
* Classifier used to register a context that is to serve as the static context.
Copy link
Contributor

Choose a reason for hiding this comment

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

Classifier used to register contexts as "singleton" capable.

/**
* Classifier used to register a context that is to serve as the static context.
*/
public static final String STATIC_CONTEXT_CLASSIFIER = "helidon-singleton-context";
Copy link
Contributor

Choose a reason for hiding this comment

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

ContextSingleton.CLASSIFIER

is better than

ContextSingleton.STATIC_CONTEXT_CLASSIFIER

At the very least we should not use "static", so either SINGLETON_CONTEXT_CLASSIFIER, CONTEXT_CLASSIFIER or just CLASSIFIER.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Version 4.x OCA Verified All contributors have signed the Oracle Contributor Agreement. testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4.x: Global values update
3 participants