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

Options to ignore the App + Assets in generic tests. #227

Merged

Conversation

ggwpez
Copy link
Contributor

@ggwpez ggwpez commented Sep 27, 2021

Adds IgnoreApp and IgnoreAssets options to make the [channel] GenericBackendTest work with the polkadot backend.

Closes #170

Copy link
Contributor

@matthiasgeihs matthiasgeihs left a comment

Choose a reason for hiding this comment

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

This looks fine, but I don't know if this is actually needed. Instead of specifying options, you can setup state1 and state2 such that they are equal on the fields that you don't want to test them.

@ggwpez
Copy link
Contributor Author

ggwpez commented Sep 28, 2021

This looks fine, but I don't know if this is actually needed. Instead of specifying options, you can setup state1 and state2 such that they are equal on the fields that you don't want to test them.

I will try it without for now and see if it works.

PS: It does not work since there is only one asset in the Polkadot backend.

Signed-off-by: Oliver Tale-Yazdi <oliver@perun.network>
@ggwpez
Copy link
Contributor Author

ggwpez commented Oct 19, 2021

Confirmed to work with perun-network/perun-polkadot-backend#3, resolving draft status.

@ggwpez ggwpez marked this pull request as ready for review October 19, 2021 09:13
matthiasgeihs
matthiasgeihs previously approved these changes Oct 20, 2021
Copy link
Contributor

@matthiasgeihs matthiasgeihs left a comment

Choose a reason for hiding this comment

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

Looks good, I was hoping we would get away without another change to the core.

I also experimented a bit with a different approach: simply checking whether the Assets are equal or not. And in this case ignoring them. This also works. A reoccurring problem seems to be that the function buildModifiedStates is supposed to only output modified states, but this may not be true depending on the inputs given. Here is my single-line fix: perun-network@556b481

I don't know. Your approach might be cleaner. But I also think conceptionally there is something wrong because if a function is called buildModifiedStates it should always output modified states (or error), and not just if the inputs are prepared in a specific way.

@matthiasgeihs
Copy link
Contributor

@ggwpez Please see my review above. I am fine with your version, but I have also an alternative suggestion. If you prefer your approach, feel free to merge, but I think you need to rebase first anyways.

@matthiasgeihs matthiasgeihs merged commit e5b2809 into hyperledger-labs:dev Oct 20, 2021
@matthiasgeihs matthiasgeihs deleted the 170-generic-ch-test branch October 20, 2021 13:46
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.

Optionalize [channel] GenericBackendTest
2 participants