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

test: fix test_liquidity_hints test case #129

Merged
merged 4 commits into from
May 9, 2022

Conversation

3nprob
Copy link

@3nprob 3nprob commented May 1, 2022

Since all fees are equal it's arbitrary which one gets chosen - but it does seem like this case consistently gets A>B>E selected.

Maybe making the fees in the channels of the graph not equal could be worth considering in order to ensure that there is a single correct answer and that the shortest path indeed does get chosen given equal lengths - but for now this should at least make it pass as-is.

Also fixes import paths so one can run individual tests by e.g. python3 lndmanage:test -m unittest discover -p 'test_pathfinding.py'

@3nprob 3nprob force-pushed the fix-test-liquidity-hints branch 2 times, most recently from e7b2dd9 to a558aad Compare May 5, 2022 04:49
@3nprob
Copy link
Author

3nprob commented May 5, 2022

Rebased on master

@bitromortac
Copy link
Owner

bitromortac commented May 6, 2022

There's something strange going on here. Tests depend on order of execution.

@bitromortac
Copy link
Owner

bitromortac commented May 6, 2022

The path weights are not equal by default even though the fees are all the same. The reason behind this is the probability factor which depends on the channel capacity, which are different. The problem here is that if the test is run in isolation or together with others, liquidity hints are loaded or not present. The correct outcome should be A -> B -> E. Need to think more about Network side effects.

Should get really rid of these side effects. Could you change it to:

    with mock.patch.object(Network, 'load_graph', return_value=None):
        network = Network(MockNode())
        network.graph = nx.MultiGraph()
        network.edges = {}
        # TODO: fix side effects of liquidity hints loading
        network.liquidity_hints = LiquidityHintMgr(MockNode.pub_key)

@3nprob 3nprob force-pushed the fix-test-liquidity-hints branch from a558aad to 9893070 Compare May 9, 2022 05:20
@3nprob
Copy link
Author

3nprob commented May 9, 2022

Ah, missed that gotcha there - and agreed on the todo!

Updated it now as requested. Also verified that each test .py completes successfully both independently and as part of a full suite run.

@bitromortac
Copy link
Owner

Thank you for pushing these investigations!

@bitromortac bitromortac merged commit 30f1a6c into bitromortac:master May 9, 2022
@3nprob 3nprob deleted the fix-test-liquidity-hints branch May 9, 2022 12:17
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.

None yet

2 participants