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

Add allowAppDeployFailures boolean property #144

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

Conversation

brideck
Copy link
Member

@brideck brideck commented Dec 18, 2024

Short description of what this resolves:

The WebSocket TCK contains some negative deployment tests that attempt to deploy applications that are expected to fail, but then need to proceed into test methods in order to check that various endpoints have not been registered. Currently, these failing applications prevent Arquillian from creating a URL suitable for testing.

Changes proposed in this pull request:

  • Create a new allowAppDeployFailures property that defaults to false, maintaining normal, expected behavior
  • Setting this new property to true will ignore any deploy exceptions that occur while processing all applications. In the case of an ignored exception, the plugin will proceed to create a best guess URL for the application that Arquillian can use for testing.

Fixes: #137

@brideck brideck requested a review from jhanders34 December 18, 2024 21:00
@brideck brideck self-assigned this Dec 18, 2024
@brideck brideck requested a review from Azquelt December 18, 2024 21:12
@brideck brideck changed the title Add checkAppTargetState boolean property Add allowAppDeployFailures boolean property Dec 19, 2024
@brideck
Copy link
Member Author

brideck commented Dec 19, 2024

Jared reminded me that I had originally proposed a different property name in the linked issue. We both like that name more than what I'd originally used here, so I've updated that and swizzled the implementation to more accurately match it.

@Azquelt
Copy link
Member

Azquelt commented Dec 19, 2024

I hate this, the TCK is broken, we can't be the only people hitting this.

I can't decide whether I think this hack is bad enough that we should instead subclass the container in the test runner and catch and swallow exception messages from the deploy method.

@brideck
Copy link
Member Author

brideck commented Dec 19, 2024

Still a hack, but we could more limit the scope by just skipping the exception particular to the case where apps haven't started.

https://github.com/OpenLiberty/liberty-arquillian/blob/main/liberty-managed/src/main/java/io/openliberty/arquillian/managed/WLPManagedContainer.java#L1280

@Azquelt
Copy link
Member

Azquelt commented Dec 23, 2024

I'm less fussed about exactly how we allow deployment to succeed when the app fails to start, and more about adding in an option that we'll have to support forever.

I don't object to the way you've implemented it, and if we really need this option, we can push ahead with it.

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.

2 participants