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

Feature/761 build and e2e test ci #768

Merged

Conversation

skosito
Copy link
Collaborator

@skosito skosito commented Nov 23, 2023

Description

Add build and e2e steps to workflow.
Force exit on e2e tests because jest detects this part as open handle:

    await new Promise((r) => setTimeout(r, 20000));

I think using --forceExit flag is ok here.

Also, I used public goerli rpc url for GOERLI_RPC_URL env var in workflow, probably better than hiding infura url, as we probably won't get rate limited. The rest of env vars i just copied from .env.sample since that is already in the repo.

Related Issue

#761

Motivation and Context

To make PR process more robust.

How Has This Been Tested

I tested on my fork.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Request to be added as a Code Owner/Maintainer

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I commit to abide by the Responsibilities of Code Owners/Maintainers.

@ognjenkurtic
Copy link
Collaborator

e2e test run logs this:

Failed execution of transaction with id 1141cc17-3d96-4cc4-aecb-50e8e4d557ec. Error: NotImplementedException: Not Implemented

It is because we are invoking storeAnchorHashOnCcsm in the ExecuteVsmCycleCommandHandler.

The test is still green though, i suspect we are not asserting everything we should assert. Could you please open an issue to resolve it separately?

@ognjenkurtic
Copy link
Collaborator

..and timeout is there to wait for one execution cycle to pass. Open for a better approach.

@skosito
Copy link
Collaborator Author

skosito commented Nov 23, 2023

e2e test run logs this:

Failed execution of transaction with id 1141cc17-3d96-4cc4-aecb-50e8e4d557ec. Error: NotImplementedException: Not Implemented

It is because we are invoking storeAnchorHashOnCcsm in the ExecuteVsmCycleCommandHandler.

The test is still green though, i suspect we are not asserting everything we should assert. Could you please open an issue to resolve it separately?

yea i assumed you are aware of this and it is some tmp not implemented thing, as this is also showing locally, i will open separate issue for that

regarding timeout, i understand why its there, but jest sees it as some handler that is hanging, and after tests are green, jest hangs with message that there is something not closed, and i identified it is that part, so i added --forceExit so github worklow doesnt hang

but i will include this in that separate issue i open, for now i dont think it should block merge of this PR. what do you think?

@skosito skosito linked an issue Nov 30, 2023 that may be closed by this pull request
@Ybittan
Copy link
Member

Ybittan commented Dec 11, 2023

@ognjenkurtic can you please review Skos' comment to unblock this PR?

Copy link
Collaborator

@Therecanbeonlyone1969 Therecanbeonlyone1969 left a comment

Choose a reason for hiding this comment

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

LGTM

@skosito skosito merged commit 543d584 into ethereum-oasis-op:main Dec 26, 2023
2 checks passed
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.

SRI - Add npm run build and e2e tests to github action
5 participants