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

#395_Fix_Falling_Unit_Tests #399

Merged

Conversation

NhoxxKienn
Copy link
Contributor

Description

This PR serves to fix the #395 issue of failing unit tests.

App Randomizer Internal Test

Location: channel/test/app_randomizer_internal_test.go

Cause: The issue stems from the lack of Comparator (Equal) for MockAppRandomizer. This leads to the test not recognizing that the default appRandomizer has already been set in the internal test.

Solution: Add an identifier to the MockAppRandomizer to compare the old and new appRandomizer.

Dialer Internal Test

Location: (wire/net/simple/dialer_internal_test.go)

Cause: Like @RmbRT has pointed out, there is a race between the init of wire/simple and backend/sim/wire, which set the RandomAddress and RandomAccount functions to be used in the internal tests.

Solution: Add the initialization of NewRandomAddress and NewRandomAccount to use the implementation from the wire/simple.

Alternative: Remove init() of wire/simple and only use the init of backend/sim/wire. This also results in passing internal tests but might cause issues for packages using wire/simple (need further investigation).

@iljabvh iljabvh self-requested a review February 20, 2024 10:06
Copy link
Contributor

@iljabvh iljabvh left a comment

Choose a reason for hiding this comment

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

LGTM

@iljabvh iljabvh merged commit e09ff52 into hyperledger-labs:main Feb 20, 2024
6 checks passed
@tinnendo tinnendo deleted the 395_fix_failing_unit_tests branch February 20, 2024 10:17
@iljabvh iljabvh mentioned this pull request Feb 22, 2024
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