This repository has been archived by the owner on Dec 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 32
#57 - Added CONTRIBUTING.md with info about tests and PRs. #61
Merged
Merged
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
7e0b3a3
#57 - Added CONTRIBUTING.md with info about tests and PR.
72a2bc2
#57 - Updated PR section.
a83da6a
#57 - Updated PR and tests sections.
920c0a2
#57 - Updated tests section.
8245be0
#57 - Added info about setUp() and tearDown().
e802e46
#57 - Removed constants rule
97d2da2
#57 - Edited rule 1.
b4b07e9
Merge branch 'develop' into #57
abdulowork File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
## 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. | ||
|
||
In addition, you must follow these rules | ||
|
||
1. Create an issue before creating a PR where you describe the problem. Your issue should | ||
be limited to only one problem. | ||
|
||
2. All PR commits must reference an issue and provide a brief commit message about the commit. | ||
Here is an example | ||
``` | ||
#123456 - Fixed an issue with unhanded client errors. | ||
|
||
``` | ||
|
||
## 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 intention. | ||
(e.g. "Returned value must be in the 1..5 range.")" | ||
) | ||
``` | ||
|
||
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. The usage of the setUp() and tearDown() methods is prohibited. | ||
|
||
4. The tests must not share any test values, such as sample input values. | ||
The only exception is the test resources, like directory path or test API URL. | ||
(These must be structured according to test rule #3) | ||
|
||
See [this](http://www.yegor256.com/2016/05/03/test-methods-must-share-nothing.html) for more information. | ||
|
||
5. 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 | ||
// | ||
// ClassNameTest.swift | ||
// | ||
// Created by FirstName LastName on DD/MM/YYY | ||
// | ||
``` | ||
|
||
6. The name of the unit test source file has to follow this structure | ||
|
||
``` | ||
ClassNameTest.swift | ||
``` | ||
|
||
7. The name of the integration test source file has to follow this structure | ||
|
||
``` | ||
ClassNameIT.swift | ||
``` | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@driver733 you mean without?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Biboran Done.
#66