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

feat: use nightly releases of js-ceramic packages #229

Merged
merged 14 commits into from
Feb 6, 2024

Conversation

smrz2001
Copy link
Contributor

@smrz2001 smrz2001 commented Feb 1, 2024

No description provided.

@smrz2001 smrz2001 self-assigned this Feb 1, 2024
Copy link

linear bot commented Feb 1, 2024

@@ -16,11 +16,11 @@ RUN npm rebuild loady
RUN npm rebuild node-jq
RUN npm rebuild sqlite3

FROM node:16-alpine as app
FROM node:20 as app
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 makes the image quite a bit larger but I was wasting time trying to figure out alpine issues so 🤷🏼

Long-term we'll be working in the new tests repo anyway.

@smrz2001 smrz2001 force-pushed the feature/ws1-1458-use-nightly-releases-in-ecs-e2e-tests branch from bd4d320 to c43d2fd Compare February 1, 2024 19:05
@smrz2001 smrz2001 force-pushed the feature/ws1-1458-use-nightly-releases-in-ecs-e2e-tests branch from c43d2fd to ee9b507 Compare February 1, 2024 20:17
@@ -16,8 +18,8 @@ const DATA2 = { data: 444 }
const DATA3 = { data: 555 }

declare global {
const ceramic: CeramicApi
const ceramicClient: CeramicApi
const ceramic: Ceramic
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be CeramicReaderWriter, not Ceramic. In most of the test configs this test is going to be using the http-client, not the core client. I think the only test that should use the Ceramic type is the state store test b/c it has to restart the node and only runs in the local_node variant with the core client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it this way because StreamReaderWriter doesn't have a did method:

    const originalDid = ceramic.did as DID

I can change it to get the DID from the client?

    const originalDid = ceramicClient.did as DID

Copy link
Contributor Author

@smrz2001 smrz2001 Feb 4, 2024

Choose a reason for hiding this comment

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

I reverted it to Ceramic because I couldn't get a test to pass, and I don't want to spend time debugging 🤷🏼 That should be a separate ticket, IMO, so I opened one.
image

Error: Can not verify signature for commit bagcqcera4ebtl3n7r6p45wpopizjc3g4tpsxcah7xnen3bokqbjqex54zwgq to stream kjzl6kcym7w8yahoezjzd0bhmriehs2flqirz9eksga9vatmxy8d4tjg9yvuuml which has controller DID did:key:z6Mko9DSC8znmyZbgQQL9u17ed5ogQSiwh24zSRtvwn6NDeT: invalid_jws: not a valid verificationMethod for issuer: did:key:z6MktamTUdbhrA7FbSqj1g3GpQrAuAQRJtySqiZ4sFdbRE9z#z6MktamTUdbhrA7FbSqj1g3GpQrAuAQRJtySqiZ4sFdbRE9z

    at Function.verifyCommitSignature (file:///home/mz/src/ceramic-integration-tests/node_modules/@ceramicnetwork/common/src/utils/signature_utils.ts:54:13)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at ModelInstanceDocumentHandler._applyGenesis (file:///home/mz/src/ceramic-integration-tests/node_modules/@ceramicnetwork/stream-model-instance-handler/src/model-instance-document-handler.ts:121:7)
    at StateManipulator._applyLog (file:///home/mz/src/ceramic-integration-tests/node_modules/@ceramicnetwork/core/src/stream-loading/state-manipulator.ts:69:17)
    at Repository._genesisFromNetwork (file:///home/mz/src/ceramic-integration-tests/node_modules/@ceramicnetwork/core/src/state-management/repository.ts:390:19)
    at file:///home/mz/src/ceramic-integration-tests/node_modules/@ceramicnetwork/core/src/state-management/repository.ts:269:19
    at file:///home/mz/src/ceramic-integration-tests/node_modules/p-queue/dist/index.js:187:36

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All tests are green now.

Copy link
Collaborator

@stbrody stbrody Feb 5, 2024

Choose a reason for hiding this comment

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

Wait, it failed the test at runtime? I literally don't see how changing a typescript type, which disappears at runtime, could possibly change runtime behavior? I would expect it possibly to fail to compile, but not to change behavior at runtime...

I'd like to look into this more, how can I run the tests to reproduce this?

Copy link
Collaborator

@stbrody stbrody Feb 5, 2024

Choose a reason for hiding this comment

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

Okay I'm assuming the runtime failures were caused by messing with the did to work around compile failures. I put together a branch here that avoids compile failures without doing any messing with the did. Can you check if this works and passes tests, and if so merge into this PR?

#230

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, it failed the test at runtime? I literally don't see how changing a typescript type, which disappears at runtime, could possibly change runtime behavior? I would expect it possibly to fail to compile, but not to change behavior at runtime...

Yes, exactly. It wasn't the change itself but likely the fact that I had to remove the line that restored the DID in afterAll that resulted in the failure.

I'd like to look into this more, how can I run the tests to reproduce this?

I was running the tests locally and seeing the same errors in CI.

I'll give the PR a shot, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, all tests are passing, thanks, @stbrody.

src/index.ts Outdated Show resolved Hide resolved
Copy link

@PaulLeCam PaulLeCam left a comment

Choose a reason for hiding this comment

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

LGTM, I'm just wondering what is the update process for the Ceramic package versions please?
Is it possible to automatically use the latest nightlies when the tests run?

@smrz2001
Copy link
Contributor Author

smrz2001 commented Feb 5, 2024

LGTM, I'm just wondering what is the update process for the Ceramic package versions please?
Is it possible to automatically use the latest nightlies when the tests run?

Thanks @PaulLeCam. Yes, good question. The update.sh script will run at the start of every test and update the dependencies to the latest nightlies. So, even if the latest package versions aren't checked in, we'll always be running tests with the latest versions and can check the versions from the logs if something fails. They just won't be committed until someone goes back in and runs the script by hand and commits the updates, but that shouldn't matter much from now on.

@smrz2001 smrz2001 requested a review from stbrody February 5, 2024 13:34
@PaulLeCam
Copy link

Thanks @PaulLeCam. Yes, good question. The update.sh script will run at the start of every test and update the dependencies to the latest nightlies. So, even if the latest package versions aren't checked in, we'll always be running tests with the latest versions and can check the versions from the logs if something fails. They just won't be committed until someone goes back in and runs the script by hand and commits the updates, but that shouldn't matter much from now on.

Great, thanks!

@smrz2001
Copy link
Contributor Author

smrz2001 commented Feb 5, 2024

Opened a separate ticket for local_client-public test flakiness.

@stbrody stbrody mentioned this pull request Feb 5, 2024
* Make types more specific

* prettier
Copy link
Collaborator

@stbrody stbrody left a comment

Choose a reason for hiding this comment

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

LGTM

@smrz2001 smrz2001 merged commit 99bc5cb into main Feb 6, 2024
1 check passed
@smrz2001 smrz2001 deleted the feature/ws1-1458-use-nightly-releases-in-ecs-e2e-tests branch February 6, 2024 14:30
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.

3 participants