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

Implement inner spec #1102

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

soonhong99
Copy link

Summary

This PR updates the setInner method in ArbitraryBuilder to use a lambda receiver (InnerSpec.() -> Unit) instead of the previous (InnerSpec) -> Unit approach. This change aligns with Kotlin DSL conventions and enhances code readability by enabling a more natural, declarative configuration style for InnerSpec.

Resolves #981 .


Description

  • Changed the setInner method signature:

    • Before:
      fun <T> ArbitraryBuilder<T>.setInner(innerSpecConfigurer: (InnerSpec) -> Unit): ArbitraryBuilder<T>
    • After:
      fun <T> ArbitraryBuilder<T>.setInner(innerSpecConfigurer: InnerSpec.() -> Unit): ArbitraryBuilder<T>
  • This update simplifies the configuration process for InnerSpec by eliminating the need for explicit lambda parameters like it:

    • Before:
      .setInner {
          it.size(1)
             .entry("key") { v ->
                 v.property("value", "expected")
             }
      }
    • After:
      .setInner {
          size(1)
          entry("key") {
              property("value", "expected")
          }
      }
  • Improved readability and consistency with idiomatic Kotlin DSL practices.


How Has This Been Tested?

  • Updated test cases in InnerSpecTest to verify the new configuration style:

    • Verified that property("value", "expected") correctly applies to CustomObject.
    • Confirmed nested InnerSpec configurations work as expected.
  • Ran all existing tests with:

    ./gradlew check
  • Results: All tests passed successfully.


Is the Document updated?

No updates are required for the documentation, as this change improves the internal API without altering its external usage significantly.


Additional Notes

This PR includes both the method signature change and corresponding test updates to ensure the new behavior is correctly validated. If further documentation or clarification is needed, please advise.

- Changed setInner from (InnerSpec) -> Unit to InnerSpec.() -> Unit
- Improved DSL readability by allowing method calls without 'it'

Signed-off-by: soonhong99 <[email protected]>
- Tested that `property("value", "expected")` is properly applied.
- Ensured that `CustomObject` is generated as part of the InnerSpec test case.

Signed-off-by: soonhong99 <[email protected]>
Copy link
Author

@soonhong99 soonhong99 left a comment

Choose a reason for hiding this comment

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

이슈와 별개로, 여쭤보고 싶은 부분이 있어 코멘트를 남깁니다!

Comment on lines +48 to +50
entry("key") {
property("value", "expected")
}
Copy link
Author

Choose a reason for hiding this comment

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

해당 property를 설정할 때 propertyName"value", value"expected"로 명시적으로 설정하고 있습니다.

그런데 .sample()을 호출하면, Fixture Monkey의 랜덤 데이터 생성 로직에 따라 CustomObject.value가 랜덤 값으로 채워지는 것 같습니다. 이 경우 "expected"라는 값은 실질적으로 적용되지 않고 다른 랜덤 값으로 덮어씌워질 가능성이 있는데요.

그렇다면, "expected" 값을 명시적으로 설정할 수 있도록 설계한 이유가 무엇인지 궁금합니다. 이 설정이 랜덤 데이터 생성 과정에서 어떤 역할을 하며, 왜 이러한 구조가 필요했는지 알고 싶습니다.

Copy link
Contributor

@seongahjo seongahjo Nov 22, 2024

Choose a reason for hiding this comment

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

랜덤 값으로 채워지는 결과가 나왔다면 receiver가 가르키는 대상이 달라지면서 기존과 동작이 달라지게 된 게 아닌가 싶은데요.
원래 기본적인 set API은 고정된 값을 설정해야할 때 사용하는 API입니다.
아마 동작이 기존과 다르게 랜덤하게 나와서 헷갈리셨을 것 같습니다.

원래 동작은 아래와 같습니다. 아래 검증문이 항상 성립하게 됩니다.

val actual = SUT.giveMeBuilder<Map<String, CustomObject>>()
            .setInner { m ->
                m.size(1)
                m.entry("key") { e ->
                    e.property("value", "expected")
                }
            }
            .sample()

        then(actual.values.first().value).isEqualTo("expected")

m의 scope 내에서는 m이 this가 되고, e 의 scope 내에서는 e가 this가 되는 방향으로 구현이 되야됩니다.

Copy link
Author

Choose a reason for hiding this comment

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

랜덤 값으로 채워지는 결과가 나왔다면 receiver가 가르키는 대상이 달라지면서 기존과 동작이 달라지게 된 게 아닌가 싶은데요. 원래 기본적인 set API은 고정된 값을 설정해야할 때 사용하는 API입니다. 아마 동작이 기존과 다르게 랜덤하게 나와서 헷갈리셨을 것 같습니다.

원래 동작은 아래와 같습니다. 아래 검증문이 항상 성립하게 됩니다.

val actual = SUT.giveMeBuilder<Map<String, CustomObject>>()
            .setInner { m ->
                m.size(1)
                m.entry("key") { e ->
                    e.property("value", "expected")
                }
            }
            .sample()

        then(actual.values.first().value).isEqualTo("expected")

m의 scope 내에서는 m이 this가 되고, e 의 scope 내에서는 e가 this가 되는 방향으로 구현이 되야됩니다.

아하 그렇군요! 그렇다면 제가 구현한 방식은 잘못된 구현방식일까요?
then(actual.values.first().value).isEqualTo("expected")
해당 then 구절이 항상 성립하는 방향으로 구현해야할까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

넵, 아래 then 구절이 성립하는 방향으로 구현해야 합니다.

InstantiatorDslSpec 구현을 참조해보시면 좋을 것 같습니다.
아래와 같이 사용할 수 있습니다.

  val actual = SUT.giveMeBuilder<JavaConstructorTestSpecs.JavaTypeObject>()
            .instantiateBy {
                constructor {
                    javaBeansProperty {
                        filter { "string" != it.name }
                    }
                }
            }
            .sample()

Copy link
Author

Choose a reason for hiding this comment

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

넵, 아래 then 구절이 성립하는 방향으로 구현해야 합니다.

InstantiatorDslSpec 구현을 참조해보시면 좋을 것 같습니다. 아래와 같이 사용할 수 있습니다.

  val actual = SUT.giveMeBuilder<JavaConstructorTestSpecs.JavaTypeObject>()
            .instantiateBy {
                constructor {
                    javaBeansProperty {
                        filter { "string" != it.name }
                    }
                }
            }
            .sample()

해당 내용 자세히 설명해주셔서 감사합니다! 말씀해주신 내용대로 동작할 수 있도록 수정하겠습니다.

@soonhong99
Copy link
Author

@seongahjo

InnerSpec.java 파일 내에 필요한 내용들을 모두 kotlin으로 리팩토링하는 과정이 필요할까요?
KotlinInstantiatorDsl.kt 파일 처럼 구현하기 위해서는 kotlin DSL 스타일을 이용해야할 것 같은데, 제 생각이 맞는지 여쭤보고 싶습니다.

@seongahjo
Copy link
Contributor

@soonhong99

InnerSpec.java 파일 내에 필요한 내용들을 모두 kotlin으로 리팩토링하는 과정이 필요할까요?

넵, 말씀주신 것처럼 kotlin으로 InnerSpec 구현체를 새로 추가해야할 것 같습니다.
일단은 작성 비용이 너무 크다면, InnerSpec을 상속하는 방향으로 진행해보시면 어떨까 합니다.

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.

Implement InnerSpec Kotlin DSL
2 participants