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.

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

Added a test for #918 #4906

Merged
merged 4 commits into from
Sep 18, 2019
Merged

Conversation

hackaugusto
Copy link
Contributor

Description

Issue #918 needs that the reveal timeout and settle timeout are validated against each other.

PR review check list

Quality check list that cannot be automatically verified.

  • Safety
  • Code quality
    • Error conditions are handled
    • Exceptions are propagated to the correct parent greenlet
    • Exceptions are correctly classified as recoverable or unrecoverable
  • Compatibility
    • State changes are forward compatible
    • Transport messages are backwards and forward compatible
  • Commits
    • Have good messages
    • Squashed unecessary commits
  • If it's a bug fix:
    • Regression test for the bug
      • Properly covers the bug
      • If an integration test is used, it could not be written as a unit test
  • Documentation
    • A new CLI flag was introduced, is there documentation that explains usage?
  • Specs
    • If this is a protocol change, are the specs updated accordingly? If so, please link PR from the specs repo.
  • Is it a user facing feature/bug fix?
    • Changelog entry has been added

@auto-assign auto-assign bot requested a review from karlb September 17, 2019 13:44
@codecov
Copy link

codecov bot commented Sep 17, 2019

Codecov Report

❗ No coverage uploaded for pull request base (develop@3e43309). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #4906   +/-   ##
==========================================
  Coverage           ?   80.73%           
==========================================
  Files              ?      119           
  Lines              ?    14425           
  Branches           ?     2227           
==========================================
  Hits               ?    11646           
  Misses             ?     2123           
  Partials           ?      656
Impacted Files Coverage Δ
raiden/api/python.py 68.33% <ø> (ø)

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 3e43309...deee1eb. Read the comment docs.

Copy link
Contributor

@karlb karlb left a comment

Choose a reason for hiding this comment

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

Potential additions:

  • explanation: why is this settle timeout too small
  • test that it does not raise an exception for larger settle timeouts, making sure that the threshold is exactly where it should be

This just explains the relation ship among the reveal and settle
timeouts, it is intendend to be read by a Raiden user.
@hackaugusto hackaugusto requested a review from karlb September 17, 2019 15:40
token_address=token_address,
partner_address=api2.address,
settle_timeout=invalid_settle_timeout,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I was suggesting to test that channel_open will succeed for settle_timeout = node1.raiden.config["reveal_timeout"] * 2 to verify that the settle_timeout validity threshold is honored in both directions.

But maybe the condition is so simple that a test for that is overkill. Add it or leave it at your own discretion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Added complementary calls for the settlement value
@hackaugusto hackaugusto merged commit 73046fb into raiden-network:develop Sep 18, 2019
@hackaugusto hackaugusto deleted the test_for_918 branch September 18, 2019 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants