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

clean up liveslots tests #10739

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

clean up liveslots tests #10739

wants to merge 2 commits into from

Conversation

turadg
Copy link
Member

@turadg turadg commented Dec 18, 2024

refs: #5575

Description

I read #5575 (comment) and thought it would be good to be on the latest version of Ava and default configuration before reworking the tests more.

This does that and also changes the four test.failing to use test that are confirmed to be the correct behavior. Since the test is of test failures, it uses a new spy instead.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

This is one package of the monorepo on Ava 6. I tried bumping them all before but ran into problems. We should tackle them all eventually,

Testing Considerations

This might solve #5575 but we won't know for a while since it's intermittent. Still it reduces variables in solving that problem.

Upgrade Considerations

none

@turadg turadg requested a review from warner December 18, 2024 21:09
@turadg turadg requested a review from a team as a code owner December 18, 2024 21:09
Copy link

cloudflare-workers-and-pages bot commented Dec 18, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6fbb689
Status: ✅  Deploy successful!
Preview URL: https://0480dd78.agoric-sdk.pages.dev
Branch Preview URL: https://5575-liveslots-gc.agoric-sdk.pages.dev

View logs

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

While I very much like switching to worker threads (if we're compatible) and the test cleanup, I'm a less convinced by the individual switch of a single package to ava@6. I'd prefer if we kept that for #9083

@turadg
Copy link
Member Author

turadg commented Dec 31, 2024

individual switch of a single package to ava@6

It is regrettable but in exploring #9081 it's going to be a while until we adopt it. Even when we can get green CI, there are some UX regressions that may require upstream contributions.

Meanwhile we have a flakey GC test, which is a more severe technical debt than packages having different versions.

@turadg turadg added automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR labels Dec 31, 2024
Copy link
Contributor

mergify bot commented Dec 31, 2024

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

@mhofman
Copy link
Member

mhofman commented Dec 31, 2024

we have a flakey GC test, which is a more severe technical debt than packages having different versions.

Sure, I just don't understand how a new version of ava is expected to fix the flakey aspect. It seems we're shooting in the dark here, and it feels that the workerThreads change is a lot more likely to have an effect than a newer version. Unless there is something specific with newer ava that is expected to help, but I don't see anything mentioned in the description.

@turadg turadg force-pushed the 5575-liveslots-gc branch from 9b64cca to ce01a46 Compare January 6, 2025 19:39
@turadg
Copy link
Member Author

turadg commented Jan 6, 2025

how a new version of ava is expected to fix the flakey aspect

Not expected. Just reducing variables in debugging.

I agree that workerThreads is the substantive change. I've removed the Ava 6 change.

I also went looking for more false test.failing and found one in dvo-test-harness.test.js. When I replace .failing with the spy, it's still failing. I suspect because the .failing was hiding an unexpected failure. I have to get back to other work so will leave this here for the next person who works on #5575 (currently unassigned).

@turadg turadg removed automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR labels Jan 6, 2025
@turadg turadg marked this pull request as draft January 6, 2025 19:42
@turadg turadg force-pushed the 5575-liveslots-gc branch from ce01a46 to 64d6653 Compare January 23, 2025 20:51
@turadg turadg marked this pull request as ready for review January 23, 2025 20:51
@turadg turadg force-pushed the 5575-liveslots-gc branch from 64d6653 to 6fbb689 Compare January 24, 2025 23:10
@turadg turadg added automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR labels Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants