-
-
Notifications
You must be signed in to change notification settings - Fork 79
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
Making the correct Parsing #890
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for eclipsefdn-adoptium-trss canceled.
|
Update the parsing depending on the jdk implementation is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add tests for this.
i had added a test case file could you check it and give me some feedbacks? |
@Sangyoon21 Thanks for the test coverage, that's a great start! It probably would be good to add a few more output variants for our common LTS's. e.g., For Java 8
I think there are some more examples in https://github.com/adoptium/api.adoptium.net/blob/main/adoptium-models-parent/adoptium-api-v3-models/src/test/kotlin/api/VersionParserTest.kt |
i had added some more test cases but i am not sure what should be the date for those different version. Could you clarify it please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks!
@Sangyoon21 You mentioned the dates still look incorrect in the UI though? |
i am not sure about it, i cannot check it on my side right now. |
Are you not able to build locally? |
@llxia - Is that something you can help with? |
if ((curRegexResult = javaVersionRegex.exec(output)) !== null) { | ||
javaVersion = removeTimestamp(curRegexResult[1]); | ||
} else { | ||
javaVersion = output; // Use the entire output if markers are missing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no javaVersionRegex
match, we have no idea where the java -version
is from and we should not handle the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case than should i use return null
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, in this case, there is no java -version output.
`; | ||
|
||
const result = parser.exactJavaVersion(java8Output); | ||
expect(result.jdkDate).toBe('2024-06-27'); // Adjust the expected date based on your requirements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we get 2024-06-27
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but i am not sure what should be the date for those different version. Could you clarify it please?
This was the part where i was not sure and had asked in the previous comment, so i had just added an random date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, there is no jdk build date in java -version
output.
To clarify, there could be 2 dates in java -version. For example:
The original issue is asking for getting the build date (not the release date). If there is no such a date, set null. We should not set a random date. |
I had fixed the changes. Please tell me if i have more things to change or update!! |
// Return null if no jdkDate is found | ||
if (!jdkDate) { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead, add else at line 56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean line 59?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, at line 56 add eles block
} else if ((curRegexResult = javaBuildDateRegex.exec(output)) !== null) { | ||
jdkDate = curRegexResult[0]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be removed
@Sangyoon21 could you rerun your test? Thanks |
Fix #888
I changed it to parse the right jdk date but however i couldn't make the right jdk date showing in the graph.