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

Feature/add specs maptocontain #1171

Closed

Conversation

simonNozaki
Copy link
Contributor

Outline

Add some specs to MapToContainInOrderOnlyKeyValueExpectationsSpec .

I made those reference to IterableToContainInOrderOnlyValuesExpectationsSpec .

Issue

#1129


I confirm that I have read the Contributor Agreements v1.0, agree to be bound on them and confirm that my contribution is compliant.

@simonNozaki simonNozaki requested a review from robstoll as a code owner July 15, 2022 02:13
}.toThrow<AssertionError> {
message {
toContainSize(2, 3)
elementSuccess(0, "1", "a", "1")
Copy link
Owner

Choose a reason for hiding this comment

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

shows only failing with report option

You are not expecting the contrary here. You should check that successful elements are not in the error report

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @robstoll

Thank you for reviewing. Asking questions you once before, how can I check that successful elements are not in the error report? I could not find the way to check specs I wrote...

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, did not get that this question was not solved yet. I remember that you were asking whether you can copy methods from IterableToContainsSepcBase to MapLikeToContainSpecBase. So yes, you totally can. You can for instance copy notToContainElement from there and adopt to map. I suggest we name the method notToContainEntry and it would suffice if it takes the key i.e it would look like follows:

fun Expect<String>.notToContainEntry(key: String): Expect<String> = ...

I left out the body on purpose, let me know if you don't want to figure this out yourself, I can also provide it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest we name the method notToContainEntry and it would suffice if it takes the key i.e it would look like follows:

Thanks to provide some example. I understood that I could write expectation by toContainFun , but sould I create something like notToContainEntry ?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, you should create notToContainEntry in MapLikeToContainSpecBase

)
}.toThrow<AssertionError> {
message {
elementSuccess(0, "1", "a", "1")
Copy link
Owner

Choose a reason for hiding this comment

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

same same

}.toThrow<AssertionError> {
message {
toContainSize(2, 1)
elementSuccess(0, "1", "a", "1")
Copy link
Owner

Choose a reason for hiding this comment

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

I guess you got it the wrong way, showOnlyFailingIfMoreExpectedElementsThan(2) is not met, hence it should show all successful expectations and all failing expectations

}
}
}
it("shows summary without report option if there are 2 expected elements because default for showOnlyFailingIfMoreExpectedElementsThan is 2") {
Copy link
Owner

Choose a reason for hiding this comment

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

default is 10 not 2. you need to state 10 expectations to be sure that it still shows the summary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robstoll

default is 10 not 2
I could not understand this... Why is the default value 10, not 2? Test data for spec is optional, I thought.

Copy link
Owner

Choose a reason for hiding this comment

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

report = {...} is optional exactly and you did not specify it here. If you leave it out then the default kicks in which is kind of:
report = { showOnlyFailingIfMoreExpectedElementsThan(10) }

}
}
}
it("shows only failing without report option if there are 3 expected elements because default for showOnlyFailingIfMoreExpectedElementsThan is 2") {
Copy link
Owner

Choose a reason for hiding this comment

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

same as above, the default is 10, so you should make 11 expectations about entries to see if only the failing are shown

}.toThrow<AssertionError> {
message {
toContainSize(2, 3)
elementSuccess(0, "1", "a", "1")
Copy link
Owner

Choose a reason for hiding this comment

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

Yes, you should create notToContainEntry in MapLikeToContainSpecBase

expect {
expect(map).toContainFun(
keyValue("a") { toEqual(1) },
report = { showOnlyFailingIfMoreExpectedElementsThan(1) }
Copy link
Owner

Choose a reason for hiding this comment

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

Didn't see this before, you should do what you describe in it i.e. pass 2 and not 1 to showOnlyFailingIfMoreExpectedElementsThan

Suggested change
report = { showOnlyFailingIfMoreExpectedElementsThan(1) }
report = { showOnlyFailingIfMoreExpectedElementsThan(2) }

Accordingly, you need to add two entry expectations to toContainFun to meet the criteria


Alternatively you can rewrite the it to:

it("shows summary with report option `showOnlyFailingIfMoreExpectedElementsThan(1)` because there are 1") {

@@ -257,6 +256,87 @@ abstract class MapToContainInOrderOnlyKeyValueExpectationsSpec(

context("report options") {
//TODO #1129 add tests, see IterableToContainInOrderOnlyValuesExpectationsSpec -> report options
context("") {
Copy link
Owner

Choose a reason for hiding this comment

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

describe here what map is the subject, i.e. use

Suggested change
context("") {
context("map $map") {

}
}
}
it("shows summary without report option if there are 2 expected elements because default for showOnlyFailingIfMoreExpectedElementsThan is 2") {
Copy link
Owner

Choose a reason for hiding this comment

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

report = {...} is optional exactly and you did not specify it here. If you leave it out then the default kicks in which is kind of:
report = { showOnlyFailingIfMoreExpectedElementsThan(10) }

@robstoll
Copy link
Owner

@simonNozaki closing this, will take your commits as basis for a new PR. Thanks for the pre-work

@robstoll robstoll closed this Jan 27, 2024
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