-
Notifications
You must be signed in to change notification settings - Fork 153
Fix regression tests (Analyze1FromSourcesV40) #151
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -179,6 +179,12 @@ | |
| <None Include="..\TestFrameworkOOB\UserFeedback.cs"> | ||
| <Link>Sources\UserFeedback.cs</Link> | ||
| </None> | ||
| <Compile Include="..\IteratorAnalysis\IteratorSimpleContract\IteratorSimpleContract.cs"> | ||
| <Link>Sources\IteratorSimpleContract.cs</Link> | ||
| </Compile> | ||
| <Compile Include="..\TestPostconditionInference\Filtering.cs"> | ||
| <Link>Sources\Filtering.cs</Link> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file causes ClousotTests not to show in Test Explorer, because it contains methods declared
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sharwell I prefer separate PR, but separate commits are fine for now. @tom-englert What do you think about @panacekcz comment? We can fix this in separate commit/PR.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These files should have been added as |
||
| </Compile> | ||
| <Compile Include="Tests.cs" /> | ||
| <None Include="..\Containers\TestContainers\ArrayWithNonNullAnalysis.cs"> | ||
| <Link>Sources\ArrayWithNonNullAnalysis.cs</Link> | ||
|
|
||
Large diffs are not rendered by default.
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.
❗ I would be comfortable with this in the test suite, but not in the application itself. We should find another way if at all possible.
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.
The other way is to fix thousands of formatting statements where the culture is missing.
Pros for this way of fixing (until all the code is culture-specific):
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.
Can we not put it in the test suite initialization code?
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.
The tests are just calling the .exe - we would need another argument to pass the culture to the other process.
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.
That would be preferable. My vote is for
/LCIDto match devenv.exe.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.
💡 What about using invariant culture if
/regressionoption is used? Possible implementation: 4c98ec5I think a more complicated
/LCIDdoes not add much value (why would you use it other than for this?).