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 test for coverage for scnerario using same ports for http and https #2209

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

jcarranzan
Copy link
Contributor

@jcarranzan jcarranzan commented Nov 21, 2024

Summary

This PR forms part of 3.15.2 backports set 1 and adds the test coverage for quarkusio/quarkus#43373 that was fixed here quarkusio/quarkus@e24f7a3

Note: Before Quarkus 3.15.2 this scenario fails and the app hangs, currently fails.

Please select the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • Dependency update
  • Refactoring
  • Backport
  • New scenario (non-breaking change which adds functionality)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@jcarranzan jcarranzan marked this pull request as ready for review November 21, 2024 08:20
Copy link
Member

@jedla97 jedla97 left a comment

Choose a reason for hiding this comment

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

I have one comment.
Also tested it with 3.15.1 and the quarkus is not terminated so the proces stay open and blocking port 8080 for any usage. I don't like as it can couse problems on other test which possibly using 8080 port. When I thinking about it I see WARNING [app] Service didn't start in PT5M minutes in log so maybe it should be done on side of framework, can you look at it?

http/vertx/src/test/resources/same-ports.properties Outdated Show resolved Hide resolved
@michalvavrik
Copy link
Member

Also tested it with 3.15.1 and the quarkus is not terminated so the proces stay open and blocking port 8080 for any usage.

Kudos!

Also tested it with 3.15.1 and the quarkus is not terminated so the proces stay open and blocking port 8080 for any usage.

Is this ever going to be run with 3.15.1? If there is not an easy fix on the FW side, you can go the other way around, exclude this from execution in 3.15.1.

@jedla97
Copy link
Member

jedla97 commented Nov 21, 2024

Is this ever going to be run with 3.15.1? If there is not an easy fix on the FW side, you can go the other way around, exclude this from execution in 3.15.1.

I don't mind leaving it as it is. This PR should run only on fixed version so not big deal.

But definitely issue with lowest priority for this would be nice as we can hit it with anywhere

@michalvavrik
Copy link
Member

Is this ever going to be run with 3.15.1? If there is not an easy fix on the FW side, you can go the other way around, exclude this from execution in 3.15.1.

I don't mind leaving it as it is. This PR should run only on fixed version so not big deal.

But definitely issue with lowest priority for this would be nice as we can hit it with anywhere

I don't think this PR should be left as it is. I think you mentioned the right thing - I just say if we know it doesn't work on 3.15.1, let's apply @DisabledOnQuarkusVersion and don't spend any time on this.

Copy link
Member

@jedla97 jedla97 left a comment

Choose a reason for hiding this comment

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

Thanks for changes, it seems good now

@jedla97 jedla97 merged commit 1c59431 into quarkus-qe:main Nov 21, 2024
7 checks passed
@jcarranzan jcarranzan mentioned this pull request Nov 21, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants