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

More consise reporting for contains.atLeast(1) #934

Merged
merged 12 commits into from
Jun 11, 2021

Conversation

wordhou
Copy link
Collaborator

@wordhou wordhou commented Jun 8, 2021

Resolves #310.

Modifies featureFactory in ContainsAssertionCreator to display a no such item was found text instead of a verbose feature assertion about the number of occurrences, specifically in the case contains.atLeast(1). Creates a descriptionNotFound abstract property on the ContainsAssertionContainer that gets overrode in its subclasses to provide the description for but no such item was found.

I created three new translatables for when an entry is not found, a value is not found, and a charSequence is not found (however they could all be the same if we chose).


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

@wordhou wordhou requested a review from robstoll as a code owner June 8, 2021 16:41
@wordhou wordhou force-pushed the drop-number-occurrences-atLeast1 branch from 5fa67a1 to f3a01ab Compare June 9, 2021 16:08
@wordhou wordhou requested a review from robstoll June 9, 2021 16:21
@codecov
Copy link

codecov bot commented Jun 9, 2021

Codecov Report

Merging #934 (6905d10) into master (9d97be5) will increase coverage by 0.01%.
The diff coverage is 97.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #934      +/-   ##
==========================================
+ Coverage   91.33%   91.34%   +0.01%     
==========================================
  Files         429      429              
  Lines        4280     4299      +19     
  Branches      215      219       +4     
==========================================
+ Hits         3909     3927      +18     
- Misses        322      323       +1     
  Partials       49       49              
Flag Coverage Δ
bbc 80.80% <97.29%> (+0.06%) ⬆️
bc 80.71% <97.29%> (+0.06%) ⬆️
current 87.57% <97.29%> (+0.03%) ⬆️
current_windows 86.64% <97.29%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s/creators/impl/ContainsObjectsAssertionCreator.kt 94.73% <93.33%> (-5.27%) ⬇️
...contains/creators/impl/ContainsAssertionCreator.kt 100.00% <100.00%> (ø)
...ators/impl/CharSequenceContainsAssertionCreator.kt 100.00% <100.00%> (ø)
...creators/impl/InAnyOrderEntriesAssertionCreator.kt 97.82% <100.00%> (+0.09%) ⬆️
.../creators/impl/InAnyOrderValuesAssertionCreator.kt 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d97be5...6905d10. Read the comment docs.

): AssertionGroup {
val count = search(multiConsumableContainer, searchCriterion)
val featureAssertion = featureFactory(count, descriptionNumberOfOccurrences)
val mismatchedAssertions = mismatches(multiConsumableContainer, searchCriterion)
val assertions = if (searchBehaviour is NotSearchBehaviour) {
listOfNotNull(createExplanatoryGroupForMismatches(mismatchedAssertions))
Copy link
Owner

Choose a reason for hiding this comment

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

IMO listOfNotNull is wrong here as we don't want an emptyList / assertions should be a non-empty list in the end. If you move the call of mismatches inside the if then you can change createExplanatoryGroupForMismatches back to return an AssertionGroup
You need to adjust InAnyOrderEntriesAssertionCreator so that you only call createExplanatoryGroupForMismatches if mismatches is not empty, but there it makes sense because we always have two assertions in the list, so we will never have an empty list

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 see, I didn't realize assertions had to be a non-empty list. When there are no sub-assertions in the list, it seems that we would rather return a descriptive assertion: assertionBuilder.createDescriptive(groupDescription, searchCriterion, trueProvider).build()? However we may need to wrap the descriptive assertion in an invisibleGroup since the searchAndCreateAssertion requires an AssertionGroup and not an Assertion.

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 it would be the simplest if we already implement the bonus you describe above. Then we don't need that featureFactory returns AssertionGroup? but can again return AssertionGroup and we escape the null-hell 😆
Isn't 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.

Yes, it seems so!

@wordhou wordhou force-pushed the drop-number-occurrences-atLeast1 branch from 05324bf to 9fb65a9 Compare June 10, 2021 22:00
@wordhou
Copy link
Collaborator Author

wordhou commented Jun 10, 2021

Sorry about the recent force-push, I believe I meant to revert rather than reset.

I've made the changes you requested, and I'm fairly sure everything works but I want to point out that the explanatory assertion in the "happy case" is never tested, and I'm not sure how to go about testing those cases.

@robstoll
Copy link
Owner

No worries about the force push. I am going to write the happy case and assign you as reviewer. This way you will see how it can be accomplished

…otlin/ch/tutteli/atrium/translations/DescriptionCharSequenceAssertion.kt
…otlin/ch/tutteli/atrium/translations/DescriptionIterableAssertion.kt
@robstoll
Copy link
Owner

@wordhou thanks for this contribution. You are on 🔥 continue like that the Atrium users will be thankful 🙂

@robstoll robstoll merged commit dac885b into robstoll:master Jun 11, 2021
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.

don't show number of occurrences for contains.inAnyOrder.atLeast(1)
2 participants