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 failure after adding transaction progress streaming #3621

Closed
3 tasks
zbuc opened this issue Jan 17, 2024 · 2 comments · Fixed by #3628
Closed
3 tasks

Test failure after adding transaction progress streaming #3621

zbuc opened this issue Jan 17, 2024 · 2 comments · Fixed by #3628
Assignees

Comments

@zbuc
Copy link
Member

zbuc commented Jan 17, 2024

Describe the bug
In PR #3559 transaction progress streaming was added and seems to work in manual testing with pcli, however the PR unexpectedly introduced test failures in CI. I tried replicating them locally but was unable to.

I spent some time paring down the test suite in that PR and this is what I found:

  • The failing test is the transaction_sweep test
  • When only the transaction_sweep test is present in CI, the smoke tests pass, indicating an inter-test conflict
  • When the test_orders test is reintroduced alongside the transaction_sweep test, the failure begins to occur

Steps to resolve

Additional Context

@conorsch conorsch self-assigned this Jan 17, 2024
@conorsch conorsch moved this to In Progress (Already claimed) in Testnets Jan 17, 2024
@erwanor
Copy link
Member

erwanor commented Jan 17, 2024

I think the url is expired

@conorsch conorsch moved this from 🗄️ Backlog to In progress in Penumbra Jan 18, 2024
@github-project-automation github-project-automation bot moved this to 🗄️ Backlog in Penumbra Jan 18, 2024
@conorsch
Copy link
Contributor

Discussion udring sync: the two failing tests are swap and sweep. I've noticed when running locally that if I remove the sweep test entirely, then swap starts failing again. Odd. That's an interaction we need to explore further.

However, #3559 remaining unmerged is blocking web team progress, so I'm going to disable sweep for now, get 3559 to green, and then continue investigation after the core logic has landed.

conorsch added a commit that referenced this issue Jan 18, 2024
Tracking resolution in [0], disabling the test for now,
so we can merge [1] and unblock the web team.

[0] #3621,
[1] #3559
conorsch added a commit that referenced this issue Jan 18, 2024
Tracking resolution in [0], disabling the test for now,
so we can merge [1] and unblock the web team.

[0] #3621,
[1] #3559
conorsch added a commit that referenced this issue Jan 18, 2024
Tracking resolution in [0], disabling the test for now,
so we can merge [1] and unblock the web team.

[0] #3621,
[1] #3559
conorsch added a commit that referenced this issue Jan 18, 2024
Turns out the spooky failure of the tx sweep integration test
was just that sweeping was taking slightly longer, and `pcli tx sweep`
was still using a naive sleep, rather than the more robust
await-confirmation logic that we've built into the planner.

This reverts commit 5b4fb16.

Closes #3621.
conorsch added a commit that referenced this issue Jan 19, 2024
Turns out the spooky failure of the tx sweep integration test
was just that sweeping was taking slightly longer, and `pcli tx sweep`
was still using a naive sleep, rather than the more robust
await-confirmation logic that we've built into the planner.

This reverts commit 5b4fb16.

Closes #3621.
@github-project-automation github-project-automation bot moved this from In progress to Done in Penumbra Jan 19, 2024
@github-project-automation github-project-automation bot moved this from In Progress (Already claimed) to Testnet 63: Rhea (Web Wallet) in Testnets Jan 19, 2024
@erwanor erwanor moved this from Testnet 63: Rhea (Web Wallet) to Testnet 65: Deimos in Testnets Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Status: Testnet 65: Deimos
Development

Successfully merging a pull request may close this issue.

3 participants