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

[WFCORE-6177] Remove test property added by EncodingPersistenceTestCase #5330

Merged
merged 1 commit into from Dec 15, 2022

Conversation

yersan
Copy link
Collaborator

@yersan yersan commented Dec 15, 2022

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Dec 15, 2022
@yersan
Copy link
Collaborator Author

yersan commented Dec 15, 2022

Hi @bstansberry , the integration Jobs are unrelated to this PR, so could you take a look and approve it if you agree so we can unlock the other PRs? I'll file a PR for 19.x too, this only affects the test suite

Copy link
Contributor

@bstansberry bstansberry left a comment

Choose a reason for hiding this comment

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

@yersan I have one suggestion. Please don't wait for further review; go ahead and merge when you're happy with the change (even now is ok.)

@@ -72,6 +72,9 @@ public void testEncodingManagementClient() throws Exception {
op = Util.getReadResourceOperation(PROPERTY_ADDR);
ModelNode result = client.executeForResult(op);
Assert.assertEquals("áéíóú", result.get(VALUE).asString());

op = Util.createRemoveOperation(PROPERTY_ADDR);
client.executeForResult(op);
Copy link
Contributor

Choose a reason for hiding this comment

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

This cleanup should happen before the assertEquals so it happens even if the assert fails.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch!, yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It won't cover all the cases, the read there is just a sanity check. If there are encoding issues reading/writing the configuration, the server even won't start on L70 ... let me see if I find a reasonable simple way to do it.

The other option to change the https://github.com/wildfly/wildfly-core/blob/main/testsuite/manualmode/src/test/java/org/jboss/as/test/manualmode/logging/AbstractLoggingTestCase.java#L324 and make it more resilent to read invalid characters

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bstansberry Using a different configuration file specific to this test will do the trick, it'll leave on the history minor "garbage" due to starting the test with a different config file, but if the tests pass, I think we could be fine with it.

@bstansberry bstansberry merged commit e5d4a12 into wildfly:main Dec 15, 2022
@bstansberry
Copy link
Contributor

Thanks @yersan

@yersan yersan deleted the WFCORE-6177 branch December 16, 2022 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes
Projects
None yet
2 participants