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

Refactoring Unit Tests #35

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Refactoring Unit Tests #35

wants to merge 5 commits into from

Conversation

srjheam
Copy link

@srjheam srjheam commented Dec 1, 2020

As previously discussed in #30 and #29 refactoring this project's tests might be a good idea

Closes #30

@srjheam srjheam self-assigned this Dec 1, 2020
@srjheam
Copy link
Author

srjheam commented Dec 1, 2020

Hi @nickmartin1ee7, what do you think about the progress so far?

@srjheam
Copy link
Author

srjheam commented Dec 1, 2020

I'm planning to:

  • Cover all the classes' methods.
  • Make TestConfig the only bridge between Tests and Fakes, e.g. [Test]TestSomething calls TestConfig which calls Fake which aswers TestConfig which deploys the object to the [Test] ([Test] ↦ TestConfig ↦ Fakes).
  • Separe test files between folders imitating the project's structure. (maybe?)

What's your opinion about it, @nickmartin1ee7?

@srjheam
Copy link
Author

srjheam commented Dec 1, 2020

  • Separe test files between folders imitating the project's structure. (maybe?)

IMHO Much better

@srjheam
Copy link
Author

srjheam commented Dec 2, 2020

  • Make TestConfig the only bridge between Tests and Fakes, e.g. [Test]TestSomething calls TestConfig which calls Fake which aswers TestConfig which deploys the object to the [Test] ([Test] ↦ TestConfig ↦ Fakes).

Progress have been make. Did I do it right? @nickmartin1ee7 @DonSagiv
It still looks a little messy.

Copy link
Member

@nickmartin1ee7 nickmartin1ee7 left a comment

Choose a reason for hiding this comment

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

There are a few issues here noted in my comments.

I like the idea of a static class that removes the issue with identical fakes being instantiated in every test though.

Code2Gether-Discord-Bot.Tests/Fakes/FakeProject.cs Outdated Show resolved Hide resolved
Code2Gether-Discord-Bot.Tests/Fakes/FakeProjectManager.cs Outdated Show resolved Hide resolved
/// <summary>
/// Represents a fake <see cref="ProjectDAL"/>. This class cannot be inherited.
/// </summary>
internal sealed class FakeProjectRepository : ProjectDAL { }
Copy link
Member

Choose a reason for hiding this comment

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

You should not inherit the DAL. The DAL will be the actual connect to a database. A fake repository should just inherit IProjectRepository

Copy link
Author

Choose a reason for hiding this comment

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

I inherited that Fake from ProjectDAL just to reuse the code, but now should I copy and paste the code of the ProjectDAL to the Fake?

Code2Gether-Discord-Bot.Tests/TestConfig.cs Outdated Show resolved Hide resolved
Code2Gether-Discord-Bot.Tests/TestConfig.cs Outdated Show resolved Hide resolved
Code2Gether-Discord-Bot.Tests/TestConfig.cs Outdated Show resolved Hide resolved
@srjheam
Copy link
Author

srjheam commented Dec 6, 2020

I just want to point out how easy it was to apply these fixes, I just had to change a few lines in TestConfig.

I got that idea from: Writing Clean Tests – It Starts From the Configuration.

@nickmartin1ee7
Copy link
Member

@srjheam We are going to wait on this PR. The #32 is going to merge soon and this will drastically affect the tests.

@Code2Gether-Discord Code2Gether-Discord locked and limited conversation to collaborators Dec 19, 2020
@nickmartin1ee7
Copy link
Member

@srjheam Would you want to update this branch, and look at this again issue again?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants