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

[closes #4956] Extend netty-http test coverage #4999

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

avano
Copy link
Contributor

@avano avano commented Jun 19, 2023

No description provided.

@avano avano force-pushed the issue-4956 branch 3 times, most recently from 18a53e6 to 7979867 Compare June 19, 2023 11:00
@avano
Copy link
Contributor Author

avano commented Jun 20, 2023

it seems that one test is flaky on windows, I'm looking at it

.map(h -> h.getKey() + ":" + h.getValue()).collect(Collectors.joining(","))));
});

from("netty-http:http://localhost:{{camel.netty-http.port}}/response").log("${body}");
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the test verifies the 200 status code, but maybe we should transform or echo the body back to the client so that we can verify that the route worked as expected.

@Override
public Map<String, String> start() {
// for JVM
System.setProperty("java.security.auth.login.config", "src/test/resources/config.jaas");
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading from the source tree will not work when the tests are run in the Quarkus Platform project.

Copy the file to target and use that as the path.

System.setProperty("java.security.auth.login.config", "src/test/resources/config.jaas");
final Map<String, String> properties = super.start();
// for native
properties.put("java.security.auth.login.config", "src/test/resources/config.jaas");
Copy link
Contributor

Choose a reason for hiding this comment

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

As per previous comment about reading from the source tree.

@jamesnetherton
Copy link
Contributor

By introducing a new netty-http itest module, we now have tests split between there and integration-tests/http. We should maybe consider moving the netty-http specific stuff our of integration-tests/http and into integration-tests/netty-http.

However, that could be non-trivial to do. So not sure if we should address it in this in this PR or do a follow-up. Maybe others have some thoughts on that.

There is also this proposal to add the HTTP tests to integration-test-groups #3087.

@avano
Copy link
Contributor Author

avano commented Jun 23, 2023

@jamesnetherton I can take a look at splitting the http tests in a separate PR

@jamesnetherton
Copy link
Contributor

@jamesnetherton I can take a look at splitting the http tests in a separate PR

Sounds good, thanks.

Do you mind rebasing with the latest work on main. Just want to make sure everything passes ok with the Quarkus 3.2.0 upgrade.

@avano
Copy link
Contributor Author

avano commented Jun 23, 2023

@jamesnetherton I can take a look at splitting the http tests in a separate PR

Sounds good, thanks.

Do you mind rebasing with the latest work on main. Just want to make sure everything passes ok with the Quarkus 3.2.0 upgrade.

after the upgrade to 3.2.0 I needed to register more classes for reflection here

@jamesnetherton jamesnetherton merged commit daae493 into apache:main Jun 26, 2023
@zbendhiba
Copy link
Contributor

thanks @avano

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.

4 participants