Skip to content

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.

/// <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?

@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

Development

Successfully merging this pull request may close these issues.

[User Story] As a Developer, I want better unit tests on logical classes to reduce the chance of re-introducing a bug

2 participants