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.

Sign up for GitHub

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

Jump to bottom

Set JDK 17 as a default JDK for representation tests #559

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from
Open

Set JDK 17 as a default JDK for representation tests #559

wants to merge 19 commits into from

Conversation

eupp
Copy link
Collaborator

@eupp eupp commented Mar 6, 2025

No description provided.

@eupp eupp requested review from ndkoval and bbrockbernd March 6, 2025 20:18
bbrockbernd
bbrockbernd reviewed Mar 6, 2025
View reviewed changes
src/jvm/test/org/jetbrains/kotlinx/lincheck_test/util/TestUtils.kt
@@ -55,53 +54,88 @@ private fun compareAndOverwrite(expectedOutputFilePrefix: String, actualOutput:
check(!expectedOutputFilePrefix.contains(".txt")) {
"Filename $expectedOutputFilePrefix should not contain a file extension (.txt)"
}
// Always overwrite jdk8 non-trace
if (testJdkVersion == TestJdkVersion.JDK_8 && !isInTraceDebuggerMode && OVERWRITE_REPRESENTATION_TESTS_OUTPUT) {
getExpectedLogFileFromSources(getFileNameFor(expectedOutputFilePrefix, TestJdkVersion.JDK_8, false)).writeText(actualOutput)
Copy link
Collaborator

Choose a reason for hiding this comment

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

When running the test on the default jdk, it would be nice if it created a file if it was not found. Might be cleaner to do that in getExpectedLogFile

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, sorry, I did not realize it was for this purpose.
I restored this behavior.

Copy link
Collaborator

@bbrockbernd bbrockbernd Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe also throw when there exists no file AND (we are not in JDK17 OR in trace mode). To encourage first running and creating the default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's allow creating the file automatically if the OVERWRITE_REPRESENTATION_TESTS_OUTPUT flag is passed.
It is easier this way to create the file on the first run.

A developer can then look at generated file or diff to verify it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, sorry, I see your point now.
If there is no "default" file, it will always create the jdk-specific file. Ok, I'll fix that.

@eupp eupp changed the base branch from master to develop March 7, 2025 00:58
@eupp eupp requested a review from bbrockbernd March 7, 2025 01:00
@eupp
Copy link
Collaborator Author

eupp commented Mar 7, 2025 •
edited
Loading

As a part of this PR, I also disabled spin-loop representation tests in trace debugger mode.
Spin-loop detection is unsupported in the trace debugger mode at the moment.
Because of this, the log files for these tests are just too long and not human-readable anyway.

bbrockbernd
bbrockbernd requested changes Mar 7, 2025
View reviewed changes
src/jvm/test/org/jetbrains/kotlinx/lincheck_test/util/TestUtils.kt
),

// next, try to pick the jdk-specific file
TestFileConfiguration(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: this can also be wrapped in if (isInTraceDebuggerMode) and the bottom two in if (testJdkVersion != DEFAULT_TEST_JDK_VERSION)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll keep it as is. The logic is already complicated enough, I would prefer to keep the number of if-s minimal.

src/jvm/test/org/jetbrains/kotlinx/lincheck_test/util/TestUtils.kt
@@ -55,53 +54,88 @@ private fun compareAndOverwrite(expectedOutputFilePrefix: String, actualOutput:
check(!expectedOutputFilePrefix.contains(".txt")) {
"Filename $expectedOutputFilePrefix should not contain a file extension (.txt)"
}
// Always overwrite jdk8 non-trace
if (testJdkVersion == TestJdkVersion.JDK_8 && !isInTraceDebuggerMode && OVERWRITE_REPRESENTATION_TESTS_OUTPUT) {
getExpectedLogFileFromSources(getFileNameFor(expectedOutputFilePrefix, TestJdkVersion.JDK_8, false)).writeText(actualOutput)
Copy link
Collaborator

@bbrockbernd bbrockbernd Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe also throw when there exists no file AND (we are not in JDK17 OR in trace mode). To encourage first running and creating the default.

eupp added 19 commits March 8, 2025 01:34
@eupp
minor refactoring in TestUtils.kt
fab9e93 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
modify representation tests file name lookup logic
1c44dcd 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
fix default JDK overwrite logic
e61c132 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
remove special handling of default JDK overwrite logic
2d688ca 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
fix trace files for default JDK (17)
bbfd20c 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
perform clean in the script before running tests
e5d1f37 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
fix trace files for all JDKs
34802a5 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
fix trace files
d02944e 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
disable spin-loop-related representation tests in trace debugger mode
7c9755d 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
update expected trace file
86943ac 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
remove unnecessary log file
7ca2ca0 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
update log files
ed5a0c7 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
put JDK 17 on the first place in the script
8b16ed3 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
change log filenames look-up strategy and updated the comments
3d2346f 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
create the file if it does not exists
f4b4e37 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
loopify getExpectedLogFile
75ab7e5 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
minor
9ea94cf 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
do not automatically create the expected log file for non-default JDK
a358912 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
add shebang to script
9164fb3 
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp eupp force-pushed the eupp/overwrite-output-script-default-jdk17 branch from a7190fa to 9164fb3 Compare March 8, 2025 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbrockbernd bbrockbernd bbrockbernd requested changes

@ndkoval ndkoval ndkoval approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eupp @ndkoval @bbrockbernd