Skip to content

Issue #4423 Chaining multiple collection with index #4480

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

Merged
merged 2 commits into from
Oct 8, 2023
Merged

Issue #4423 Chaining multiple collection with index #4480

merged 2 commits into from
Oct 8, 2023

Conversation

crabstars
Copy link
Contributor

@crabstars crabstars commented Oct 4, 2023

Fixes #4423 for master

First Error: System.InvalidOperationException : Stack empty We've addressed the "Stack empty" error. The issue was with how certain operators were being processed. To fix it, we adjusted the LeftPrecedence when we encounter an IndexerOperator.
Explanation: in the function ReduceOperatorStack the And will not be reduced with the IndexerOperator. This will then fail in NUnit.Framework.Constraints.ConstraintBuilder.Resolve for the BinaryOperator.Reduce because it tries to pop the stack two times and will then fail.

Second Error: Default indexer accepting arguments < 0 > was not found on SomeTests+SomeStuff We believe this isn't a real error. It looks like @azygis is trying to access a collection in a specific way. He's currently using a collection of SomeStuff, but it seems he should be using a collection that contains other collections of SomeStuff.

@crabstars
Copy link
Contributor Author

@dotnet-policy-service agree company=Relaxdays

1 similar comment
@crabstars
Copy link
Contributor Author

@dotnet-policy-service agree company=Relaxdays

@crabstars crabstars changed the title Fix Issue https://github.com/nunit/nunit/issues/4423 Fix Issue #4423 Oct 5, 2023
@crabstars crabstars changed the title Fix Issue #4423 Issue #4423 Chaining multiple collection with index Oct 5, 2023
Copy link
Member

@stevenaw stevenaw left a comment

Choose a reason for hiding this comment

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

Thanks @crabstars !

I've pulled your changes and can see your new test passes with the changes and fails without. I've tried using the example from the original issue and it seems to still fail.

I think this is a good step forward though, and could merge on its own too. Did you want to pursue making the test from the originating issue work as well or would you like us to merge this as-is?

@azygis
Copy link

azygis commented Oct 7, 2023

@stevenaw question is which assert fails. The second problem hasn't been fixed as is mentioned in the PR. I don't really have a strong opinion on the second problem though, since .One.ItemAt(0) kinda makes not a lot of sense, since ItemAt(n) is of course goint to be just one.

If the first problem still fails, that ain't good.

@crabstars
Copy link
Contributor Author

Thank you for the responses. The first issue shouldn't be failing. I executed the NOT OK 1 from the reported problem, and it passed. @stevenaw, did it fail when you ran the test on that branch? If it succeeded, I believe we should proceed with the merge.

@crabstars
Copy link
Contributor Author

@azygis if you want that One.ItemAt(0).Property(nameof(SomeStuff.Name)).EqualTo("Name 1") is going to work you need something like this

var items = new SomeStuff[][]
            {
               new SomeStuff[]
                {
                new("Name 1")}
            };

If you dont know why, I can try to explain it in more detail

@stevenaw
Copy link
Member

stevenaw commented Oct 8, 2023

Thanks for clarifying @azygis @crabstars
I'll double check exactly where the failure was to confirm

@azygis
Copy link

azygis commented Oct 8, 2023

@azygis ah no, I understand. I don't even know why I reported the second not-ok. It's actually fine, just a brain freeze when I wrote that assert.

@OsirisTerje OsirisTerje merged commit 3630a95 into nunit:master Oct 8, 2023
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.

Chaining multiple collection asserts with index
4 participants