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.

Sign up for GitHub

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

Jump to bottom

contribution guide: Add guidelines for testing #1696

Merged
merged 3 commits into from
Jul 18, 2018
Merged

contribution guide: Add guidelines for testing #1696

merged 3 commits into from
Jul 18, 2018

Conversation

ValarDragon
Copy link
Contributor

@ValarDragon ValarDragon commented Jul 16, 2018 •
edited
Loading

Indicates to use require's and asserts, and to use table driven
tests, with error messages as described in #1664.

Closes #1664

I don't think any of the below are relevant.

  • Updated all relevant documentation (docs/)
  • Updated all relevant code comments
  • Wrote tests
  • Updated CHANGELOG.md
  • Updated cmd/gaia and examples/

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

ValarDragon and others added 2 commits July 16, 2018 14:42
@ValarDragon @rigelrozanski
Merge pull request #1691: Table driven test indexes
4b68899 
* types: Switch table driven test error messages to new format

Make table driven tests in /types follow the format described in #1664

* typos / lower case errors

* lower case, not sentences

* lower case, not sentences
@ValarDragon
contribution guide: Add guidelines for testing
1edfb89 
Indicates to use require's and asserts, and to use table driven
tests, with error messages as described in #1664.

Closes #1664
@codecov
Copy link

codecov bot commented Jul 16, 2018 •
edited
Loading

Codecov Report

Merging #1696 into develop will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1696      +/-   ##
===========================================
+ Coverage    62.35%   62.37%   +0.01%     
===========================================
  Files          120      120              
  Lines         7119     7117       -2     
===========================================
  Hits          4439     4439              
+ Misses        2429     2427       -2     
  Partials       251      251

@cwgoes cwgoes changed the base branch from master to develop July 16, 2018 19:54
@cwgoes
Copy link
Contributor

cwgoes commented Jul 16, 2018

Retargeted to develop, we're back to Gitflow.

@ValarDragon
Copy link
Contributor Author

ValarDragon commented Jul 16, 2018 •
edited
Loading

This should still be pointed at master I think. (Since we need the update on github default branch)
If we're not merging this to master though, please don't squash these commits together.

rigelrozanski
rigelrozanski reviewed Jul 16, 2018
View reviewed changes
CONTRIBUTING.md Outdated
We expect tests to use `require` or `assert` rather than `t.Skip` or `t.Fail`,
unless there is a reason to do otherwise.
We prefer to use [table driven tests](https://github.com/golang/go/wiki/TableDrivenTests)
where applicable.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd specify where this is applicable.. I'd hate for somebody to take some nice simple non-table driven tests and attempt to fit them into a table because of this comment. Maybe something along the lines of:
If a test describes a reasonably complex situation it should be isolated in it's own test function (aka. TestXxx. Where testing is fairly simple we prefer the tests to grouped through the use of [table driven tests](https://github.com/golang/go/wiki/TableDrivenTests)
+where applicable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure thats the right boundary for table driven / not table driven. I think the better boundary is whether you're testing a function under multiple scenarios or just a single scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense

@cwgoes
Copy link
Contributor

cwgoes commented Jul 16, 2018

This should still be pointed at master I think. (Since we need the update on github default branch)

Default branch is back to develop now.

Will avoid squashing commits.

@rigelrozanski
Copy link
Contributor

ugh- yeah I guess we shouldn't of merged to master in #1691 - let's just merge this to develop as per the old ways

@rigelrozanski rigelrozanski merged commit 14759ed into develop Jul 18, 2018
@rigelrozanski rigelrozanski deleted the dev/update_contribution_guide_for_testing branch July 18, 2018 03:17
@rigelrozanski
Copy link
Contributor

merged without squash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rigelrozanski rigelrozanski rigelrozanski approved these changes

@cwgoes cwgoes Awaiting requested review from cwgoes

@ebuchman ebuchman Awaiting requested review from ebuchman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ValarDragon @cwgoes @rigelrozanski