- 
  Notifications
 
You must be signed in to change notification settings  - Fork 1.3k
 
CSHARP-5540: Fix exception when using AsQueryable().Last() #1649
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
Conversation
 
 
 ...slators/ExpressionToExecutableQueryTranslators/LastMethodToExecutableQueryTranslatorTests.cs
 
 Outdated
 
 Show resolved
 Hide resolved
 
  
 
 ...slators/ExpressionToExecutableQueryTranslators/LastMethodToExecutableQueryTranslatorTests.cs
 
 Outdated
 
 Show resolved
 Hide resolved
 
  
 
 ...slators/ExpressionToExecutableQueryTranslators/LastMethodToExecutableQueryTranslatorTests.cs
 
 Outdated
 
 Show resolved
 Hide resolved
 
  
 
 ...slators/ExpressionToExecutableQueryTranslators/LastMethodToExecutableQueryTranslatorTests.cs
 
 Show resolved
 Hide resolved
 
  
 
 ...slators/ExpressionToExecutableQueryTranslators/LastMethodToExecutableQueryTranslatorTests.cs
 
 Show resolved
 Hide resolved
 
 There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't return null. It is an empty sequence. See renaming and refactoring suggested below.
 
 
 ...slators/ExpressionToExecutableQueryTranslators/LastMethodToExecutableQueryTranslatorTests.cs
 
 Outdated
 
 Show resolved
 Hide resolved
 
  
 
 ...slators/ExpressionToExecutableQueryTranslators/LastMethodToExecutableQueryTranslatorTests.cs
 
 Show resolved
 Hide resolved
 
  
 
 ...slators/ExpressionToExecutableQueryTranslators/LastMethodToExecutableQueryTranslatorTests.cs
 
 Outdated
 
 Show resolved
 Hide resolved
 
 @rstam I've changed the code following your suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to have the expectedStages variable?
All other tests are written like this:
AssertStages(
 stages,
 """{ $sort : { _id : 1 } }""",
 """{ $group : { _id : null, _last : { $last : "$$ROOT" } } }""",
 """{ $replaceRoot : { newRoot : "$_last" } }"""); 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also leave a blank between the translation assertions and the result assertions because they are asserting different things.
This is really two tests in one:
- That the translation works as we expected it to
 - That the resulting MQL works on the server and returns the expected result
 
I know that the general recommendation is that every test should test one thing only, but we have chosen to be flexible in this case because otherwise we'd have twice as many tests, and each pair of tests would have identical setup.
 
 
 ...slators/ExpressionToExecutableQueryTranslators/LastMethodToExecutableQueryTranslatorTests.cs
 
 Show resolved
 Hide resolved
 
  
 
 ...slators/ExpressionToExecutableQueryTranslators/LastMethodToExecutableQueryTranslatorTests.cs
 
 Outdated
 
 Show resolved
 Hide resolved
 
  
 
 ...slators/ExpressionToExecutableQueryTranslators/LastMethodToExecutableQueryTranslatorTests.cs
 
 Outdated
 
 Show resolved
 Hide resolved
 
  
 
 ...slators/ExpressionToExecutableQueryTranslators/LastMethodToExecutableQueryTranslatorTests.cs
 
 Outdated
 
 Show resolved
 Hide resolved
 
 There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
LGTM
Please update the PR's title to be readable as a part of release notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use single quote for fields, and then there will be no need for extra double-quotes outside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a single quote ' inside of the string, then it will be no need to have multiple quotes:
"{ $group : { _id : null, _last : { $last : '$$ROOT' } } }"
No description provided.