Skip to content

Conversation

@panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Mar 31, 2024

What changes were proposed in this pull request?

The pr aims to improve UT related to structured logs, including: LoggingSuiteBase, StructuredLoggingSuite and PatternLoggingSuite.

Why are the changes needed?

Enhance readability and make it more elegant.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

  • Manually test.
  • Pass GA.

Was this patch authored or co-authored using generative AI tooling?

No.

@panbingkun
Copy link
Contributor Author

panbingkun commented Mar 31, 2024

cc @gengliangwang @dtenedor @amaliujia @cloud-fan @HyukjinKwon @dongjoon-hyun

@panbingkun panbingkun marked this pull request as ready for review March 31, 2024 03:46
def msgWithMDCAndException: LogEntry = log"Error in executor ${MDC(EXECUTOR_ID, "1")}."

def expectedPatternForBasicMsg(level: String): String
def expectedPatternForBasicMsg(level: Level): String
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using 'Level' instead of 'String' may be more reasonable

def expectedPatternForMsgWithMDCAndException(level: Level): String

test("Basic logging") {
val msg = "This is a log message"
Copy link
Contributor Author

@panbingkun panbingkun Mar 31, 2024

Choose a reason for hiding this comment

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

msg and basicMsg are duplicated, we can make full use of basicMsg.

def basicMsg: String = "This is a log message"

}

override def expectedPatternForMsgWithMDCAndException(level: Level): String = {
compactAndToRegexPattern(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is too long,
Show it in pretty json to developers,
This can also eliminate the comment scalastyle:off line.size.limit

override def afterAll(): Unit = Logging.enableStructuredLogging()

override def expectedPatternForBasicMsg(level: String): String =
s""".*$level PatternLoggingSuite: This is a log message\n"""
Copy link
Contributor Author

@panbingkun panbingkun Mar 31, 2024

Choose a reason for hiding this comment

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

Let's make full use of the variable className to eliminate hard coding text PatternLoggingSuite

override def expectedPatternForBasicMsg(level: String): String =
s"""\\{"ts":"[^"]+","level":"$level","msg":"This is a log message","logger":"$className"}\n"""
private val jsonMapper = new ObjectMapper().registerModule(DefaultScalaModule)
private def compactAndToRegexPattern(json: String): String = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In testing, this method compactAndToRegexPattern will ultimately be used to compact the output JSON and convert it into a regex pattern for matching and validation.


// Returns the first line in the log file that contains the given substring.
protected def captureLogOutput(f: () => Unit): String = {
// Return the newly added log contents in the log file after executing the function `f`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Obviously, the using of the function is:
Return the newly added log contents in the log file after executing the function f

@gengliangwang
Copy link
Member

Thanks, merging to master

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.

2 participants