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

Leave hot signer unlocked after adding account #1339

Merged
merged 5 commits into from
Jan 17, 2023

Conversation

Jamchello
Copy link
Contributor

No description provided.

test/main/signers/hot/RingSigner/index.test.js Outdated Show resolved Hide resolved
main/signers/hot/SeedSigner/index.js Outdated Show resolved Hide resolved
main/signers/hot/RingSigner/index.js Show resolved Hide resolved
mholtzman
mholtzman previously approved these changes Jan 13, 2023
@mholtzman mholtzman dismissed their stale review January 13, 2023 13:25

tests are failing

@goosewobbler goosewobbler added enhancement New feature or request UX hot signers labels Jan 13, 2023
@goosewobbler goosewobbler changed the title leave unlocked when adding account Leave hot signer unlocked after adding account Jan 14, 2023
expect(signer.id).not.toBe(undefined)
expect(store(`main.signers.${signer.id}.id`)).toBe(signer.id)
done()
})
} catch (e) {
done(e)
}
}, 2000)
}, 10_000)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mholtzman looks like the tests failed because this test timed out, which then lead to the signer not being assigned. I have set a longer timeout here as this prevents similar failures

Copy link
Collaborator

Choose a reason for hiding this comment

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

is this happening locally or only in CI? I think this was an already existing problem in our Github actions flow so we should probably just fix the tests at some point to prevent these flaky failures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, only in the CI

@@ -43,7 +43,9 @@ describe('Ring signer', () => {

afterAll(() => {
clean()

if (signer.status !== 'locked') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

after all the tests are run we should know what state the signer is in. what's this check for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in case there is an unfortunate failure in one of the test cases, I was thinking that this would precent the really long hanging build (looks like last one hung for ~6hrs?

expect(signer.addresses.length).toBe(100)
expect(store(`main.signers.${signer.id}.id`)).toBe(signer.id)
done()
})
} catch (e) {
done(e)
}
}, 7_500)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a specific change we made that is causing these tests to take so long?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is because the unlock is now taking place in the same function call as the creation as far as I can tell

@Jamchello Jamchello merged commit 73a4226 into develop Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hot signers UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants