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

Replace ExpectedException with Assert.Throws in unit tests #227

Closed
55 tasks done
atifaziz opened this issue Jan 2, 2017 · 6 comments
Closed
55 tasks done

Replace ExpectedException with Assert.Throws in unit tests #227

atifaziz opened this issue Jan 2, 2017 · 6 comments

Comments

@atifaziz
Copy link
Member

atifaziz commented Jan 2, 2017

ExpectedException has been removed starting with NUnit 3. Unit tests need to be adjusted to use Assert.Throws instead, which is the recommended and better way.

Meanwhile #226 adds a local [ExpectedException] so tests using that attribute can compile and run unchanged with NUnit 3. The local [ExpectedException] implementation should be removed when migration to using Assert.Throws is complete.

Test files that need addressing are:

  • AcquireTest.cs
  • AssertCountTest.cs
  • AssertTest.cs
  • AtLeastTest.cs
  • BatchTest.cs
  • CartesianTest.cs
  • ConcatTest.cs
  • ConsumeTest.cs
  • CountByTest.cs
  • DistinctByTest.cs
  • EndsWithTest.cs
  • EquiZipTest.cs
  • ExceptByTest.cs
  • ExcludeTest.cs
  • FoldTest.cs
  • ForEachTest.cs
  • FullGroupJoinTest.cs
  • GenerateTest.cs
  • GroupAdjacentTest.cs
  • IncrementalTest.cs
  • IndexTest.cs
  • InterleaveTest.cs
  • LagTest.cs
  • LeadTest.cs
  • MaxByTest.cs
  • MinByTest.cs
  • NestedLoopTest.cs
  • PadTest.cs
  • PairwiseTest.cs
  • PartitionTest.cs
  • PermutationsTest.cs
  • PipeTest.cs
  • PreScanTest.cs
  • PrependTest.cs
  • RandomSubsetTest.cs
  • RandomTest.cs
  • RepeatTest.cs
  • RunLengthEncodeTest.cs
  • SegmentTest.cs
  • SingleOrFallbackTest.cs
  • SkipUntilTest.cs
  • SortedMergeTest.cs
  • StartsWithTest.cs
  • SubsetTest.cs
  • TagFirstLastTest.cs
  • TakeEveryTest.cs
  • TakeLastTest.cs
  • TakeUntilTest.cs
  • ToDataTableTest.cs
  • ToDelimitedStringTest.cs
  • TraceTest.cs
  • TraverseTest.cs
  • WindowedTest.cs
  • ZipLongestTest.cs
  • ZipShortestTest.cs
@fsateler
Copy link
Member

fsateler commented Jan 3, 2017

This will break most pending PRs (does the fix for #226 do so too?). Rebasing on top of current master will probably be required for most PRs. To mitigate a bit this problem, usage of ExpectedException should be advised against during review.

@atifaziz
Copy link
Member Author

atifaziz commented Jan 3, 2017

The PRs won't break because of [ExpectedException] because that has been added privately to the test project until this issue is closed and it can be completely removed (preventing others from using it in the future). If TestCase is being used then they'll break. Rebasing or adjusting before merging is best. However, changing the few PRs that are open right now is peanuts compared to the 55 files in the test code base that are still using [ExpectedException].

@atifaziz
Copy link
Member Author

atifaziz commented Jan 4, 2017

Since this entails a lot of manual work, it might be interesting to try and tackle this more smartly by automating the following steps via Roslyn:

  1. Load up and parse each test file
  2. Find methods decorated with [ExpectedException]
  3. Extract the exception type as the first argument to [ExpectedException]
  4. Wrap the existing body into a statement lambda that is then passed to Assert.Throws<T> and use the exception type discovered in the previous step for T
  5. Remove the [ExpectedException] attribute
  6. Write back the new syntax tree to the file

@GeorgeVovos
Copy link
Contributor

I've updated the code.
Shall I delete the Attribute?

@atifaziz
Copy link
Member Author

atifaziz commented Jan 6, 2017

@GeorgeVovos Cool. Do you have a PR to submit for review?

@GeorgeVovos
Copy link
Contributor

@atifaziz I do now...
#233

Let me now if there are any issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants