Skip to content

DOCKER-464 Added json logging test#222

Merged
tl-blom-xenit merged 10 commits intomasterfrom
DOCKER-464
May 16, 2025
Merged

DOCKER-464 Added json logging test#222
tl-blom-xenit merged 10 commits intomasterfrom
DOCKER-464

Conversation

@tl-blom-xenit
Copy link
Contributor

@tl-blom-xenit tl-blom-xenit commented May 7, 2025

Added integration tests that check if the logging in the alfresco container is JSON logging and contains the required fields.
Currently only checks the logs of type application. Access and SQL logs are not included in the test.

Signed-off-by: Tim-Lukas Blom <tim-lukas.blom@xenit.eu>
Signed-off-by: Tim-Lukas Blom <tim-lukas.blom@xenit.eu>
Signed-off-by: Tim-Lukas Blom <tim-lukas.blom@xenit.eu>
Signed-off-by: Tim-Lukas Blom <tim-lukas.blom@xenit.eu>
Signed-off-by: Tim-Lukas Blom <tim-lukas.blom@xenit.eu>
@tl-blom-xenit tl-blom-xenit self-assigned this May 7, 2025
Signed-off-by: Tim-Lukas Blom <tim-lukas.blom@xenit.eu>
@tl-blom-xenit tl-blom-xenit marked this pull request as ready for review May 7, 2025 13:45
systemProperty 'alfresco.port.internal', '8080'
systemProperty 'flavor', "${project.alfresco.flavor}"
systemProperty 'version', "${project.alfresco.version.major}" + "." + "${project.alfresco.version.minor}"
systemProperty('alfresco_image_name', "${calcRepository(project.alfresco.flavor,true)}:${calcTags(project.alfresco.version).last()}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Store the image name in a variable and use it in 2 places instead of duplicating the logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: Tim-Lukas Blom <tim-lukas.blom@xenit.eu>
@kpiot123 kpiot123 requested a review from thijslemmens May 12, 2025 13:16
kpiot123
kpiot123 previously approved these changes May 12, 2025
return result;
}

private boolean isCorrectJsonLogs(String logs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not checking if the logs are correct json. Why don't you just json parse every line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't feel the need to test if the log line was actual valid JSON, because this would mean that the logging provider would've returned broken logging. So to keep it simple just checking the presence of the required keys seemed enough. I'll change it to use JSON parsing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm nitpicking here, but we are still not testing if it valid JSON. We only test the first line that contains "type": "application".

Also, if this returns false, it does not have to mean that JSON_LOGGING was correctly set to false.

Signed-off-by: Tim-Lukas Blom <tim-lukas.blom@xenit.eu>
Signed-off-by: Tim-Lukas Blom <tim-lukas.blom@xenit.eu>
thijslemmens
thijslemmens previously approved these changes May 16, 2025
Copy link
Contributor

@thijslemmens thijslemmens left a comment

Choose a reason for hiding this comment

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

I would avoid the Thread.sleep and use Awaitility instead. That way you can wait until you have x amount of logs.

Signed-off-by: Tim-Lukas Blom <tim-lukas.blom@xenit.eu>
@tl-blom-xenit tl-blom-xenit merged commit aa92dc8 into master May 16, 2025
23 checks passed
@tl-blom-xenit tl-blom-xenit deleted the DOCKER-464 branch May 16, 2025 09:05
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.

3 participants