Skip to content

Test Github.Sync.installation/1 event #1379

Merged
begedin merged 5 commits into
developfrom
chore/test-github-sync-installation
Mar 16, 2018
Merged

Test Github.Sync.installation/1 event #1379
begedin merged 5 commits into
developfrom
chore/test-github-sync-installation

Conversation

@zacck-zz
Copy link
Copy Markdown
Member

What's in this PR?

Added a test block for Github.Sync.installation event

Make sure any changes to code include changes to documentation.

References

Fixes #1373

Progress on: #

@zacck-zz zacck-zz force-pushed the chore/test-github-sync-installation branch from 8d2ebea to 224c80f Compare February 19, 2018 16:12
@zacck-zz
Copy link
Copy Markdown
Member Author

zacck-zz commented Feb 27, 2018

Some clauses defined seem difficult or impossible to reach so their tests were omitted
- {:error, :validation_error_on_syncing_installation, Changeset.t()}
- {:error, :validation_error_on_marking_installation_processed, Changeset.t()}
- {:error, :unexpected_transaction_outcome, any}
- {:error, :validation_error_on_deleting_removed_repos, {list, list}}

  • {:error, :validation_error_on_syncing_existing_repos, {list, list}}
    However, if these clauses can be caused by some updates upstream we should cover them with tests

@zacck-zz zacck-zz force-pushed the chore/test-github-sync-installation branch from 24e9506 to e960094 Compare February 27, 2018 09:59
@zacck-zz zacck-zz changed the title [WIP] to test the Github.Sync.installation/1 event Test Github.Sync.installation/1 event Feb 27, 2018
Copy link
Copy Markdown
Contributor

@begedin begedin left a comment

Choose a reason for hiding this comment

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

Just a small change here and good to go 👍

- {:error, :validation_error_on_marking_installation_processed, Changeset.t()}
- {:error, :unexpected_transaction_outcome, any}
However, if these clauses can be caused by some updates upstream we should cover them with tests
"""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@doc is used generally for public function documentation only (def). This should just be a simple comment:

# Some clauses defined seem difficult or impossible to reach so their tests were omitted
#  - {:error, :validation_error_on_syncing_installation, Changeset.t()}
#  - {:error, :validation_error_on_marking_installation_processed, Changeset.t()}
#  - {:error, :unexpected_transaction_outcome, any}
# However, if these clauses can be caused by some updates upstream we should cover them with tests

Also, "omitted" is spelled with one m

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed this!

@zacck-zz zacck-zz requested a review from begedin March 13, 2018 15:34
@zacck-zz
Copy link
Copy Markdown
Member Author

🤘

@begedin begedin merged commit 744e2ec into develop Mar 16, 2018
@begedin begedin deleted the chore/test-github-sync-installation branch March 16, 2018 08:32
@begedin
Copy link
Copy Markdown
Contributor

begedin commented Mar 16, 2018

Merged 👍

Thank you for your time and work @zacck

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Write tests for GitHub.Sync.installation_event/1

3 participants