Updates to our IOperation testing#26862
Merged
jaredpar merged 4 commits intodotnet:masterfrom May 15, 2018
Merged
Conversation
No longer require conditional compilation in order to run the full IOperation test suite.
Keeping this offline for now until we get to the root of the Spanish machine issues.
Member
Author
|
FYI @dotnet/roslyn-compiler |
jcouv
approved these changes
May 15, 2018
333fred
reviewed
May 15, 2018
| { | ||
| public BaseTupleBinaryOperatorExpression(BinaryOperatorKind operatorKind, SemanticModel semanticModel, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue, bool isImplicit) | ||
| : base(OperationKind.BinaryOperator, semanticModel, syntax, type, constantValue, isImplicit) | ||
| : base(OperationKind.TupleBinaryOperator, semanticModel, syntax, type, constantValue, isImplicit) |
Member
There was a problem hiding this comment.
Seems like an unrelated bug fix, can we break it out into a separate change? #Closed
Member
There was a problem hiding this comment.
Ah, nevermind, I see that these were in a separate commit. To satisfy my curiosity, why were these bug fixes made in a separate branch, and not in master directly?
In reply to: 188370704 [](ancestors = 188370704)
Member
Author
There was a problem hiding this comment.
I actually thought they'd take longer and be more disruptive. Hence we did them in a branch instead. Turns out it was much easier than I thought based on the number of failures we started with.
333fred
reviewed
May 15, 2018
| { | ||
| // Bind the expression for error recovery, but discard all new diagnostics | ||
| iterationErrorExpression = BindExpression(node.Variable, new DiagnosticBag()); | ||
| if (iterationErrorExpression.Kind == BoundKind.DiscardExpression) |
333fred
reviewed
May 15, 2018
| Diagnostic(ErrorCode.ERR_MustDeclareForeachIteration, "_").WithLocation(6, 18) | ||
| ); | ||
|
|
||
| var tree = comp.SyntaxTrees.Single(); |
333fred
approved these changes
May 15, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The IOperation verification includes running an extra validation walker over our trees for virtually every compilation done in our unit tests. This validation is not suitable for every test, due to the computations needed, and hence is disabled where necessary.
Previously this ran via a conditional compilation approach. This made testing IOperation a bit awkward. Had to first do a recompile and then run a test. This PR changes the IOperation leg to instead look for an environment variable:
%ROSLYN_TEST_IOPERATION%.I was a bit leery about using an environment variable here. But there really isn't any other supported mechanism for feeding options to tests via xunit. Other tests in our system have decided to use environment variables as well hence this isn't adding a new mechanism, rather unifying our tests.
This won't yet move the IOperation testing into our normal PR flow. Want to get our Spanish tests stable first before we add another leg into the mix.
Note: these changes have all already gone through full review. This is just merging features/optest into master.