Verify log file rotation for periods longer than day #1946

gtroitsk · 2024-08-25T22:01:11Z

Summary

Verifies issue quarkusio/quarkus#40016

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

jedla97 · 2024-08-26T07:28:39Z

@gtroitsk It's look good to me! Just would be better also checking if the file is created with that suffix?

michalvavrik · 2024-08-26T12:08:08Z

@gtroitsk It's look good to me! Just would be better also checking if the file is created with that suffix?

Let's go further, let's check content as it adds better coverage while this is very much like unit test. IMHO we don't want to test isolated bugs if we can have something similar to what users do. And if they enable logging to file, they are likely to desire there is a content.

michalvavrik

In addition to my previous comment, also please rebase on current main to fix CI.

logging/jboss/src/test/java/io/quarkus/ts/logging/jboss/DefaultMinLogLevelIT.java

github-actions · 2024-08-27T22:18:01Z

Following jobs contain at least one flaky test: 'PR - Linux - JVM build - Latest Version'

michalvavrik · 2024-08-28T08:10:01Z

logging/jboss/src/test/java/io/quarkus/ts/logging/jboss/DefaultMinLogLevelIT.java

+                        throw new UncheckedIOException(e);
+                    }
+                })
+                .anyMatch(line -> line.contains(EXPECTED_LOG_MESSAGE));


this succeeds if at least one of the files has expected line, I don't know how many files you expect. We have io.quarkus.test.utils.FileUtils, have you considered using them to cut the verbosity (try catches etc.)? I'd expect using io.quarkus.test.utils.FileUtils#findTargetFile(java.nio.file.Path, java.lang.String) and io.quarkus.test.utils.FileUtils#loadFile(java.lang.String) would cut your code to three lines.

I expect 3 files according to configuration. Check if each of rotated files contain the log message now use FileUtils.loadFile

okay, 3? in that case previous check was incorrect because you didn't check that each file contains that line. You did test that any did. I'll comment elsewhere on this expectation.

michalvavrik · 2024-08-28T08:10:52Z

@gtroitsk it looks good, I still think you could refactor it bit, but please consider it rather unimportant.

ok

michalvavrik · 2024-08-28T09:01:51Z

logging/jboss/src/test/java/io/quarkus/ts/logging/jboss/DefaultMinLogLevelIT.java

+
+        Awaitility.await().atMost(10, TimeUnit.SECONDS).until(() -> {
+            File[] rotatedFiles = logDir.toFile().listFiles((dir, name) -> name.startsWith(LOG_FILE_NAME));
+            return rotatedFiles != null && rotatedFiles.length > 1;


yeah, so, you expect 3 files but are content with finding one? I understood your comment that you want to find this line in 3 files, so I expect you need 3 files. Won't the test be flaky if you wait just for one and hope others are stored in the meanwhile?

Tbh I don't think it was wrong as with backup logs you can't know how many will be there. Someone can update quarkus.log.file.rotation.max-file-size and this test can have only one backup log.

sorry, that is not acceptable; someone cannot just update max-file-size because this is part of the test; if he updates it, the test must fail

@jedla97 configuration properties are either required for test in their current value to test a feature, or we should drop them

I played with it before review and the test failed. But there can be value where the test will pass. Anyway the test is updated to check for maximum number of files so it should be fine

Sure, I have no idea whether this test is stable. I commented on changing property for no reason. by looking at this test I think it is weird I don't understand why it is separated to 3 files and max-file-size refers to memory size, not lines. I somehow expected @gtroitsk have it worked out :-)

michalvavrik · 2024-08-28T09:02:13Z

logging/jboss/src/test/java/io/quarkus/ts/logging/jboss/DefaultMinLogLevelIT.java

+        List<Path> rotatedFiles = Files.list(logDir)
+                .filter(path -> path.getFileName().toString().matches(LOG_FILE_NAME + expectedSuffix)).toList();
+
+        assertFalse(rotatedFiles.isEmpty(), "Rotated log file with expected suffix not found.");


this looks like you are looking for single file...

github-actions · 2024-08-28T11:19:12Z

Following jobs contain at least one flaky test: 'PR - Linux - JVM build - Latest Version'

jedla97

I just have two minor comments in this last update.

logging/jboss/src/test/java/io/quarkus/ts/logging/jboss/DefaultMinLogLevelIT.java

logging/jboss/src/main/resources/application.properties

github-actions · 2024-08-28T12:53:50Z

Following jobs contain at least one flaky test: 'PR - Linux - JVM build - Latest Version'

quarkusio/quarkus#40016

jedla97

Thanks for changes

github-actions · 2024-08-28T17:18:45Z

Following jobs contain at least one flaky test: 'PR - Linux - JVM build - Latest Version'

michalvavrik · 2024-08-28T17:25:19Z

The flaky test is related to Kafka.

michalvavrik · 2024-08-28T17:26:41Z

logging/jboss/src/test/java/io/quarkus/ts/logging/jboss/DefaultMinLogLevelIT.java

+        List<Path> rotatedFiles = Files.list(logDir)
+                .filter(path -> path.getFileName().toString().matches(LOG_FILE_NAME + expectedSuffix)).toList();
+
+        assertFalse(rotatedFiles.isEmpty(), "Rotated log files with expected suffix were not found.");


For record @jedla97 , I still think this condition doesn't make sense. First there is waiting for max-backup-index + 1=3 and now it's satisfied with 1

gtroitsk requested a review from jedla97 August 25, 2024 22:04

michalvavrik previously requested changes Aug 26, 2024

View reviewed changes

gtroitsk marked this pull request as draft August 26, 2024 16:28

gtroitsk force-pushed the verify-log-file-rotation branch 3 times, most recently from e98b116 to ac1ea5f Compare August 27, 2024 19:32

gtroitsk marked this pull request as ready for review August 27, 2024 19:33

gtroitsk requested a review from michalvavrik August 27, 2024 19:36

michalvavrik reviewed Aug 27, 2024

View reviewed changes

logging/jboss/src/test/java/io/quarkus/ts/logging/jboss/DefaultMinLogLevelIT.java Outdated Show resolved Hide resolved

michalvavrik reviewed Aug 27, 2024

View reviewed changes

logging/jboss/src/test/java/io/quarkus/ts/logging/jboss/DefaultMinLogLevelIT.java Show resolved Hide resolved

github-actions bot added the triage/flaky-test Signal that flaky tests were detected during CI run label Aug 27, 2024

gtroitsk force-pushed the verify-log-file-rotation branch from ac1ea5f to e08ae96 Compare August 28, 2024 07:34

gtroitsk added the triage/backport-3.8 label Aug 28, 2024

michalvavrik reviewed Aug 28, 2024

View reviewed changes

gtroitsk force-pushed the verify-log-file-rotation branch 2 times, most recently from e1262c6 to ce56d26 Compare August 28, 2024 08:50

michalvavrik reviewed Aug 28, 2024

View reviewed changes

gtroitsk force-pushed the verify-log-file-rotation branch from ce56d26 to 1b79f2e Compare August 28, 2024 12:11

jedla97 requested changes Aug 28, 2024

View reviewed changes

logging/jboss/src/test/java/io/quarkus/ts/logging/jboss/DefaultMinLogLevelIT.java Outdated Show resolved Hide resolved

logging/jboss/src/main/resources/application.properties Show resolved Hide resolved

Verify log file rotation for periods longer than day

f1c9939

quarkusio/quarkus#40016

gtroitsk force-pushed the verify-log-file-rotation branch from 1b79f2e to f1c9939 Compare August 28, 2024 13:03

jedla97 approved these changes Aug 28, 2024

View reviewed changes

michalvavrik merged commit 538724d into quarkus-qe:main Aug 28, 2024
14 checks passed

michalvavrik reviewed Aug 28, 2024

View reviewed changes

fedinskiy added the triage/backport-3.15 label Aug 29, 2024

This was referenced Aug 30, 2024

[3.8] Verify log file rotation for periods longer than day #1956

Merged

[3.15] Verify log file rotation for periods longer than day #1957

Merged

gtroitsk deleted the verify-log-file-rotation branch August 30, 2024 09:14

fedinskiy removed the triage/backport-3.15 label Aug 30, 2024

gtroitsk removed the triage/backport-3.8 label Aug 30, 2024

michalvavrik removed the triage/flaky-test Signal that flaky tests were detected during CI run label Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify log file rotation for periods longer than day #1946

Verify log file rotation for periods longer than day #1946

gtroitsk commented Aug 25, 2024 •

edited

Loading

jedla97 commented Aug 26, 2024

michalvavrik commented Aug 26, 2024

michalvavrik left a comment

github-actions bot commented Aug 27, 2024

michalvavrik Aug 28, 2024

gtroitsk Aug 28, 2024

michalvavrik Aug 28, 2024

michalvavrik commented Aug 28, 2024 •

edited

Loading

michalvavrik Aug 28, 2024

jedla97 Aug 28, 2024

michalvavrik Aug 28, 2024

michalvavrik Aug 28, 2024

jedla97 Aug 28, 2024

michalvavrik Aug 28, 2024

michalvavrik Aug 28, 2024

github-actions bot commented Aug 28, 2024

jedla97 left a comment

github-actions bot commented Aug 28, 2024

jedla97 left a comment

github-actions bot commented Aug 28, 2024

michalvavrik commented Aug 28, 2024

michalvavrik Aug 28, 2024

Verify log file rotation for periods longer than day #1946

Verify log file rotation for periods longer than day #1946

Conversation

gtroitsk commented Aug 25, 2024 • edited Loading

Summary

Checklist:

jedla97 commented Aug 26, 2024

michalvavrik commented Aug 26, 2024

michalvavrik left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalvavrik commented Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 28, 2024

jedla97 left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 28, 2024

jedla97 left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 28, 2024

michalvavrik commented Aug 28, 2024

Choose a reason for hiding this comment

gtroitsk commented Aug 25, 2024 •

edited

Loading

michalvavrik commented Aug 28, 2024 •

edited

Loading