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.

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

[ICTL-908] Test Compiler for Kotlin #282

Merged
merged 59 commits into from
Jul 25, 2024

Conversation

Frosendroska
Copy link
Contributor

@Frosendroska Frosendroska commented Jun 30, 2024

Description of changes made

Detailed PR explanation can be found in https://youtrack.jetbrains.com/issue/ICTL-908/Test-Compiler-for-Kotlin
Partially solves the following issue #295

TestAssempler diagram

image

Why is merge request needed

This is the last step for Kotlin support.

Other notes

Closes #ICTL908

What is missing?

Probably, there are some bugs in the whole implementation. They will be fixed during the review or while testing the TestSpark using TestSpark.

  • I have checked that I am merging into correct branch

@Frosendroska Frosendroska self-assigned this Jul 8, 2024
@Frosendroska Frosendroska marked this pull request as ready for review July 8, 2024 16:41
@Frosendroska Frosendroska added Urgent Urgant PR improvement New feature labels Jul 8, 2024
@pderakhshanfar pderakhshanfar self-requested a review July 19, 2024 12:33
@Frosendroska
Copy link
Contributor Author

Some changes raised from the review of this PR will be addressed in:

  1. [Language-related-refactoring] TopButtonsPanelStrategy
  2. [Language-related-refactoring] TestClassBuilderHelper refactoring

Copy link
Collaborator

@Vladislav0Art Vladislav0Art left a comment

Choose a reason for hiding this comment

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

Great that you added some tests! All fine!
I added a small comment about refactoring (about replacing single-method factories with static factory methods inside the base classes). It can be dedicated to another PR.

Comment on lines +8 to +17
class TestBodyPrinterFactory {
companion object {
fun createTestBodyPrinter(language: SupportedLanguage): TestBodyPrinter {
return when (language) {
SupportedLanguage.Kotlin -> KotlinTestBodyPrinter()
SupportedLanguage.Java -> JavaTestBodyPrinter()
}
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

[Thinking out load here; not for this PR but worth investing effort at]:

Every [Something]Factory class that we have often contains a single method create[Something](language). We can turn the Factory Pattern into the Factory Method Pattern.

These create[Something]() static methods can be moved into the base abstract classes/interfaces (e.g., in this case we can place createTestBodyPrinter() static method into TestBodyPrinter interface (and maybe rename to just create()). This will allow us to remove a single entity from the codebase, namely TestBodyPrinterFactory. It'll reduce the code complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in #305

Comment on lines 11 to 15
class TestCompilerFactory {
companion object {
fun createJavacTestCompiler(
fun createTestCompiler(
project: Project,
junitVersion: JUnitVersion,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can also turn this factory into a factory method within the TestCompiler interface.

Comment on lines +10 to +13
class TestSuiteParserFactory {
companion object {
fun createJUnitTestSuiteParser(
jUnitVersion: JUnitVersion,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one can also be turned into a factory method.

@Frosendroska Frosendroska merged commit edfdba2 into development Jul 25, 2024
3 checks passed
@arksap2002 arksap2002 deleted the ebraun/improvements/compiler-for-kotlin branch August 20, 2024 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement New feature Urgent Urgant PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants