Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
aspiers
changed the title
Aim to prevent duplicate objects in the same DB which breaks tests based on snapshots.
There was a problem hiding this comment.
Pull request overview
This pull request attempts to fix CI test failures by skipping failing tests, updating the permission model from a two-level (read/write) to a four-level (read/create/update/delete) system, and making various infrastructure and configuration changes.
Changes:
- Updated permission model across all tests and source code from
{read, write}to{read, create, update, delete} - Skipped multiple failing test suites with
describe.skip()andit.skip() - Added majestic package for Jest UI, updated GitHub Actions runners, and modified workflow configurations
Reviewed changes
Copilot reviewed 44 out of 46 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Added majestic Jest UI tool and supporting dependencies |
| packages/sds/tests/*.test.ts | Updated permission model from read/write to read/create/update/delete across all tests |
| packages/sds/tests/*.test.ts | Skipped failing test suites with describe.skip/it.skip |
| packages/sds/src/permission-manager/index.ts | Added owner access checks and changed audit log ordering from timestamp to ID |
| packages/sds/src/oauth/federated-token-validator.ts | Removed tokenType parameter from validateToken method |
| packages/sds/src/index.ts | Added schemas export |
| packages/dev-env/src/*.ts | Moved mockNetworkUtilities call timing and added SDS lexicon registration |
| .github/workflows/*.yaml | Changed runners from ubuntu-latest/ubuntu-22.04 to blacksmith runners and .nvmrc for Node version |
| packages/sds-demo/src/*.tsx | Updated React Query API usage and removed unused imports |
| packages/dev-infra/docker-compose.yaml | Removed deprecated version field |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }) | ||
|
|
||
| it('Allows to sign-in trough OAuth', async () => { | ||
| it.skip('Allows to sign-in trough OAuth', async () => { |
There was a problem hiding this comment.
The spelling of "trough" should be "through". This appears in the test description.
| import { SeedClient, TestNetworkNoAppView, basicSeed } from '@atproto/dev-env' | ||
| import { verifyRepoCar } from '@atproto/repo' | ||
| import { AppContext, scripts } from '../dist' | ||
| import type { AppContext } from '../../pds/dist/context' |
There was a problem hiding this comment.
Importing from ../../pds/dist/context creates a tight coupling between the SDS package and the PDS package's compiled output. This is fragile and could break if the PDS package structure changes. Consider creating a shared types package or exporting these types from the PDS package's main entry point instead of reaching into its dist folder.
| import { TestNetworkNoAppView } from '@atproto/dev-env' | ||
| // Importing from `dist` to circumvent circular dependency typing issues | ||
| import { AccountDb } from '../dist/account-manager/db' | ||
| import type { AccountDb } from '../../pds/dist/account-manager/db' |
There was a problem hiding this comment.
Importing from ../../pds/dist/account-manager/db creates a tight coupling between the SDS package and the PDS package's compiled output. This is fragile and could break if the PDS package structure changes. Consider creating a shared types package or exporting these types from the PDS package's main entry point instead of reaching into its dist folder.
| }) | ||
| permissionManager = new SdsPermissionManager( | ||
| network.sds.ctx.accountManager.db, | ||
| network.sds.ctx.accountManager.db as unknown as Database<DatabaseSchema>, |
There was a problem hiding this comment.
Using as unknown as type assertions to bypass type checking is a code smell that indicates the types are not properly aligned. The use of double type assertions suggests the actual type and expected type are incompatible. This should be fixed by properly typing the database or adjusting the permission manager constructor to accept the correct type.
This comment has been minimized.
This comment has been minimized.
As a result of seeing failures like this in different packages:
packages/sds test: ● Test suite failed to run
packages/sds test: thrown: "Exceeded timeout of 60000 ms for a hook.
packages/sds test: Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."
packages/sds test: 24 | })
packages/sds test: 25 |
packages/sds test: > 26 | afterAll(async () => {
packages/sds test: | ^
packages/sds test: 27 | await network.close()
packages/sds test: 28 | })
packages/sds test: 29 |
packages/sds test: at afterAll (tests/sds-endpoint-logic.test.ts:26:3)
packages/sds test: at Object.describe (tests/sds-endpoint-logic.test.ts:8:1)
packages/sds test: PASS SDS tests/sync/sync.test.ts
packages/sds test: PASS SDS tests/handles.test.ts
packages/sds test: PASS SDS tests/proxied/procedures.test.ts
packages/sds test: Test Suites: 1 failed, 1 skipped, 4 passed, 5 of 6 total
packages/sds test: Tests: 4 skipped, 63 passed, 67 total
packages/sds test: Snapshots: 0 total
packages/sds test: Time: 77.922 s
packages/sds test: Ran all test suites.
packages/sds test: Jest did not exit one second after the test run has completed.
packages/sds test: This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue.
Seems like this is an issue with undici and jest.
aspiers
changed the title
Try to fix tests