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

Add actual but not expected items in the contains exactly matcher #106

Merged

Conversation

brunsa2
Copy link
Contributor

@brunsa2 brunsa2 commented Dec 1, 2020

Contain exactly would previously not show any diff if there were things in the actual list that weren't in the expected list, but this is still a failure case.

@mcmire
Copy link
Collaborator

mcmire commented Dec 1, 2020

I think this might be a solution for #96. So at a glance this looks good, but let me try this out in a test project to make sure.

@brunsa2 brunsa2 force-pushed the contain_exactly_show_actual_not_expected branch 2 times, most recently from fdb347e to dee4d6f Compare December 13, 2020 22:32
@brunsa2
Copy link
Contributor Author

brunsa2 commented Dec 13, 2020

I'm still going to try to figure out what's going on here (I haven't exactly figured out how match works yet), but it appears that through match it's not going to contains_exactly. @mcmire Do you know where the match pathway goes and what differ or whatnot is used here?

@mcmire mcmire self-assigned this Dec 14, 2020
@mcmire
Copy link
Collaborator

mcmire commented Dec 18, 2020

Hey @brunsa2, sorry for the delay in responding. The tests you changed in match_matcher_spec look like they have to do with a_collection_including and not a_collection_containing_exactly. So I don't think you need to change those. However, you're in the right file, it just might be more beneficial to add to or change the tests for a_collection_containing_exactly. Does that make sense?

@brunsa2
Copy link
Contributor Author

brunsa2 commented Dec 22, 2020

I'm not following. I'd expect contains_exactly and a_collection_containing_exactly to behave the same, but it appears that they are not following the same code path (or else the changes to collection_containing_exactly.rb should reflect in the match tests for a collection containing.

@brunsa2
Copy link
Contributor Author

brunsa2 commented Dec 22, 2020

Thoughts: put the extra actual values in order rather than at the end.

  • In favor: might be helpful if the diff matches the actual array order for debugging purposes
  • In opposition: this matcher is meant to be order-independent and this would not reinforce that.
expect [a, b, c] to contain exactly [a, c]
  a
  c
+ b

vs.

expect [a, b, c] to contain exactly [a, c]
  a
+ b
  c

(I originally though the second was better but now like the first, as it currently stands, better again)

@brunsa2 brunsa2 force-pushed the contain_exactly_show_actual_not_expected branch 2 times, most recently from 1a23435 to c5cf6fb Compare December 23, 2020 18:09
@brunsa2
Copy link
Contributor Author

brunsa2 commented Dec 23, 2020

@mcmire CI is now failing before starting. I've tried twice.

@mcmire
Copy link
Collaborator

mcmire commented Dec 23, 2020

@brunsa2 Sorry about that. I just found out recently that a new release of RSpec broke the build. To add to this, Travis is switching over to a paid model, so CI builds take forever to run. I'm working on switching over to GitHub Actions and fixing the gem at the same time. Stay tuned!

@mcmire
Copy link
Collaborator

mcmire commented Dec 23, 2020

@brunsa2 Okay, the build is fixed and CI is now running against GitHub Actions instead of Travis. Try rebasing your branch against master!

@brunsa2 brunsa2 force-pushed the contain_exactly_show_actual_not_expected branch from c5cf6fb to 435ebdc Compare December 24, 2020 16:55
@brunsa2 brunsa2 force-pushed the contain_exactly_show_actual_not_expected branch from 435ebdc to 39772fd Compare December 24, 2020 20:49
@brunsa2
Copy link
Contributor Author

brunsa2 commented Dec 24, 2020

Unless you're interested in changes from #106 (comment) I think this is ready, but there's a en extra "test" line waiting for status (not associated to a particular test config) that's holding off CI.

@brunsa2
Copy link
Contributor Author

brunsa2 commented Jan 4, 2021

@mcmire if you missed it

@brunsa2
Copy link
Contributor Author

brunsa2 commented Jan 11, 2021

@mcmire

1 similar comment
@brunsa2
Copy link
Contributor Author

brunsa2 commented Feb 4, 2021

@mcmire

@mcmire
Copy link
Collaborator

mcmire commented Feb 4, 2021

I apologize. I have been tending to other things. I will take a look at this tonight.

@mcmire
Copy link
Collaborator

mcmire commented Feb 5, 2021

I don't know why it's showing the build as not completed yet, because it is. Re: changing how the diff is presented, I don't know if I have an opinion on that right now. I can see both sides, but I feel like it's something that can be come back to. Anyway, I'll merge this. Sorry for the long wait and thank you!

@mcmire mcmire merged commit e81eca6 into splitwise:master Feb 5, 2021
@brunsa2 brunsa2 deleted the contain_exactly_show_actual_not_expected branch February 23, 2021 20:31
@brunsa2
Copy link
Contributor Author

brunsa2 commented Feb 23, 2021

Thanks :)

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.

None yet

2 participants