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
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
## Pull Requests

All changes, no matter how trivial, must be done via pull request. Commits
should never be made directly on the `develop` branch. Prefer rebasing over
merging `develop` into your PR branch to update it and resolve conflicts.
`master` branches are for releases only.

## Tests

When writing a unit test, please be sure to follow these rules.

1. You test has this structure

```
expect{
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.

)
```

However, test failure description is not necessary,
if the class you are testing implements the `Error` protocol.

2. If you are creating fake classes/decorators for a test, make sure
that these classes are named as `FakeXXXX`.

3. Constant values that are used in the test follow this structure
```
private let CONTANT = "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.


4. Your test has proper documentation, which explains what class it is testing
in the format specified below

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

//
// ClassNameTest.swift
//
// Created by FirstName LastName on DD/MM/YYY
//
```

5. The name of the unit test source file has to follow this structure

```
ClassNameTest.swift
```

6. The name of the integration test source file has to follow this structure

```
ClassNameIT.swift
```


1 change: 1 addition & 0 deletions Example/Web3Swift.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@
50C3ECB5548832459AC51D59 /* Pods */,
B8B89B3A7E7096470B1FDD04 /* Frameworks */,
584FAC786109413F2D9921CE /* Extensions */,
C2E095FBE1BB2235A75CDA84 /* CONTRIBUTING.md */,
);
sourceTree = "<group>";
};
Expand Down