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

Added a test to the TCK to ensure observer method injection points are also validated #423

Merged
merged 1 commit into from
Oct 6, 2019

Conversation

ljnelson
Copy link
Contributor

@ljnelson ljnelson commented May 9, 2019

This pull request adds a test that ensures that observer method injection points are also validated.

Suppose you have this:

private static final void onStartup(@Observes @Initialized(ApplicationScoped.class) final Object event,
                                    @ConfigProperty("this.property.does.not.exist") final String value) {
    // do something
}

…you want a DeploymentException to be thrown in this case. I hope this is useful.

Signed-off-by: Laird Nelson ljnelson@gmail.com

…e also validated

Signed-off-by: Laird Nelson <ljnelson@gmail.com>
@eclipse-microprofile-bot
Copy link
Contributor

Can one of the admins verify this patch?

Copy link
Member

@Emily-Jiang Emily-Jiang left a comment

Choose a reason for hiding this comment

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

@ljnelson are you still working on this PR?

@ljnelson ljnelson marked this pull request as ready for review August 14, 2019 04:29
@ljnelson
Copy link
Contributor Author

@Emily-Jiang Sorry for the delay in responding. I've removed the "draft" status of this PR and it is ready to merge.

@ljnelson
Copy link
Contributor Author

@Emily-Jiang Curious if you were going to merge this?

@ljnelson
Copy link
Contributor Author

@Emily-Jiang Just another two-week check to see if this was going to be merged now that it is approved.

@kenfinnigan
Copy link
Contributor

@ljnelson, unfortunately, there's a need to revert the master to a near 1.3 release state, which is likely what's holding this up from being merged

@ljnelson
Copy link
Contributor Author

Totally makes sense; thanks!

Copy link
Member

@Emily-Jiang Emily-Jiang left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @ljnelson !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants