Skip to content
This repository has been archived by the owner on Dec 15, 2023. It is now read-only.

#57 - Added CONTRIBUTING.md with info about tests and PRs. #61

Merged
merged 8 commits into from
Feb 27, 2018

Conversation

driver733
Copy link
Contributor

As per #57
I have added CONTRIBUTING.md with directions on writing tests and creating PRs.

@0crat 0crat added the scope label Feb 24, 2018
@0crat
Copy link

0crat commented Feb 24, 2018

Job #61 is now in scope, role is REV

CONTRIBUTING.md Outdated
## Pull Requests

All changes, no matter how trivial, must be done via pull request. Commits
should never be made directly on the `master` branch. Prefer rebasing over
Copy link
Member

Choose a reason for hiding this comment

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

Let's use develop branch instead. Master branch is only for stable code and releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rockfridrich
Copy link
Member

rockfridrich commented Feb 24, 2018

Codecov Report

Merging #61 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #61   +/-   ##
========================================
  Coverage    92.37%   92.37%           
========================================
  Files           59       59           
  Lines         1509     1509           
  Branches        26       26           
========================================
  Hits          1394     1394           
  Misses         115      115

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2a5241...b4b07e9. Read the comment docs.

//
// This source file is part of the Web3Swift.io open source project
// Copyright 2018 The Web3Swift Authors
// Licensed under Apache License v2.0
Copy link
Member

Choose a reason for hiding this comment

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

This is the default file template for this project, so in most cases, it should be applied automatically.
Let's describe examples how contributors need to write tests purposes and assets descriptions before the test?
@Biboran what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rockfridrich I think that assert descriptions are extra, since we already have test failure descriptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rockfridrich Making a template for test case sounds like a good idea but I have no clue how we can make such template useful. Lets instead draft up strict rules for the linter and mention them in this file.

Test intention (description) should prefer the description: dependency rather than a comment before test method.

Copy link
Contributor Author

@driver733 driver733 Feb 26, 2018

Choose a reason for hiding this comment

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

@Biboran Can we ban test methods without :description in swift lint?
Like here

Copy link
Contributor

Choose a reason for hiding this comment

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

@driver733 you mean without?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Biboran Yes, I have edited the comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

@driver733 I think it should be possible and we should do it. Lets make it a separate issue.

Copy link
Contributor Author

@driver733 driver733 Feb 26, 2018

Choose a reason for hiding this comment

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

@Biboran Done.
#66

CONTRIBUTING.md Outdated

3. Constant values that are used in the test follow this structure
```
private let FILE_PATH: SomeType = "Some test value for test purposes."
Copy link
Contributor

@abdulowork abdulowork Feb 26, 2018

Choose a reason for hiding this comment

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

@driver733 I don't see a reason for snake case naming. The majority of swift community uses camel case. Is there a reason you want to use it?

Design wise we should keep all constants in the test methods themselves without introducing them as variables/constants in method body or test parameter. If the test method is hard to understand without using a constant it's a good sign of the bad object design.

For dependencies such as metamask infura node there should be an object in advanced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Biboran I agree with you on the camel case point and will make a new commit to address that.

However, I believe that we should have constasts in test classes. This article explains it well.

  1. Our test case is easier to read because the magic numbers are replaced with constants which are named properly.
  2. Our test case is easier to maintain because we can change the values of constants without making any changes to the actual test case.
  3. It is easier to write new tests for the registerNewUserAccount() method of the RepositoryUserService class because we can use constants instead of magic numbers. This means that we don’t have to worry about typos.

Copy link
Contributor

Choose a reason for hiding this comment

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

@driver733 take a look at this article.

  1. Our test case is easier to read because the magic numbers are replaced with constants which are named properly.

If our tests are hard to read because of constants the design of the object is wrong. Purpose of the dependency shouldn't be conveyed based on its constant/variable name.

  1. Our test case is easier to maintain because we can change the values of constants without making any changes to the actual test case.

This is addressed in the above stated article.

  1. It is easier to write new tests for the registerNewUserAccount() method of the RepositoryUserService class because we can use constants instead of magic numbers. This means that we don’t have to worry about typos.

This seems to me like a bad design of the test case or a missing scope. Typos are not a problem. You want to know your test failed because of a typo. If it doesn't fail because of a typo it's probably because of a poor object design or the constant is completely unrelated to the behavior of the testing case.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should instead prohibit setUp and tearDown methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Biboran I agree with the article you have referenced. I am talking about the constants, such as test API key or test directory path. These can and are often used in different tests in one tests class.

See an example here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@driver733 there is no need to reference API key anywhere. We should have a relevant JSON structure provided or a hardcoded object (such as with metamask infura API Network). The example you provided is not relevant because in our case every call to API (aka JSON RPC) is wrapped by the RemoteProcedure interface.

CONTRIBUTING.md Outdated
```
private let FILE_PATH: SomeType = "Some test value for test purposes."
```
In other words, do not hardcode the constants in the test methods or constructors.
Copy link
Contributor

Choose a reason for hiding this comment

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

@driver733 What is the problem that this rule is going to solve for test cases?

There are many cases in cryptography/encoding objects where you want to hardcode constants known in advanced. Can you show a better design for these cases?

These is definitely a problem with DI in iOS unit test cases design if the problem you want to solve is DI. I didn't find a way to supply dependencies to the test cases via a "main" function but we can use JSON structures supplied in the bundle to inject some specific dependencies.

Copy link
Contributor Author

@driver733 driver733 Feb 26, 2018

Choose a reason for hiding this comment

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

@Biboran The problem of low maintainability. Test constants have a name, which describes them. In addition they can be referenced several times in different places throughout the test. Without it, it is not easy what does the constant represent, what it should be used for and how.

Copy link
Contributor

Choose a reason for hiding this comment

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

@driver733 if you see that a hardcoded constant causes ambiguity assume it is a bad OO design and make a relevant issue. Assume same for code duplication. As of now I don't see a reason for this rule to exist.

Also added info about shared test values.
@driver733
Copy link
Contributor Author

@Biboran I have addressed your concerns.

@driver733
Copy link
Contributor Author

@Biboran I have removed the constants rule

CONTRIBUTING.md Outdated
ClassYouAreTesting().value()
}.To(
expectedValue,
"Description of the test failure."
Copy link
Contributor

@abdulowork abdulowork Feb 26, 2018

Choose a reason for hiding this comment

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

@driver733 How about making that a test intention rather than test failure? Tests can cascade exceptions and computational errors for a number of unknown reasons which you might not be aware of but test intention lets you know what was supposed to happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Biboran I agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Biboran Rule 1 has been edited.

@driver733
Copy link
Contributor Author

@Biboran Can we merge this?

@abdulowork abdulowork merged commit d338df8 into zeriontech:develop Feb 27, 2018
@0crat 0crat removed the scope label Feb 27, 2018
@0crat
Copy link

0crat commented Feb 27, 2018

The job #61 is now out of scope

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.

4 participants