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

Move e2e tests from a dedicated crate to tests/ at the workspace root #1566

Merged
merged 21 commits into from
Feb 19, 2025

Conversation

agryaznov
Copy link
Contributor

A small enhancement regarding integration tests:

  1. We declare a "nominal" timechain package in the workspace Cargo.toml.
  2. This allows us to get rid of a dedicated crate for e2e tests, and have them living in tests/ directory, which is neat and in line with general convention for integration tests.

@agryaznov agryaznov requested a review from penumbra23 February 19, 2025 11:25
@agryaznov
Copy link
Contributor Author

@penumbra23 let's come up with the CI settings reliable enough to be accepted for all the future PRs

@agryaznov agryaznov marked this pull request as ready for review February 19, 2025 11:27
Copy link
Collaborator

@dvc94ch dvc94ch left a comment

Choose a reason for hiding this comment

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

don't forget to remove the e2e-tests folder...

@agryaznov
Copy link
Contributor Author

@penumbra23 could port mappings break things for other ci jobs? If so we can rm it for now and disable the e2e flow until that's solved

@haider-rs
Copy link
Contributor

i guess both chain-2-evm and chain-3-evm is mapped to same port. If we change one to something else it shouldnt be an issue.

@agryaznov
Copy link
Contributor Author

agryaznov commented Feb 19, 2025

i guess both chain-2-evm and chain-3-evm is mapped to same port. If we change one to something else it shouldnt be an issue.

hmm to me it looks like they're mapped to different ones: 8545 and 8546
(otherwise it wouldn't work locally, but it works)

@haider-rs
Copy link
Contributor

correct my bad.

@agryaznov
Copy link
Contributor Author

For now let's merge this w the new e2e flow off, and without port mappings, until we figure out proper CI setting on the runners for that with @penumbra23 (tbd in a follow-up PR)

@penumbra23
Copy link
Contributor

One thing that can be done is export the port but avoid a fixed one:

ports:
- 9944

In this way the OS will assign a free one each time.

@agryaznov
Copy link
Contributor Author

One thing that can be done is export the port but avoid a fixed one:

ports:
- 9944

In this way the OS will assign a free one each time.

this particular one has been there for a quite a while (was not touched in recent PRs) so I keep it as is here

@agryaznov agryaznov merged commit a49bdae into development Feb 19, 2025
12 checks passed
@FlorianFranzen
Copy link
Collaborator

FlorianFranzen commented Feb 20, 2025

@agryaznov This actually added an invalid workflow file still failing CI in some status messages. Just though to let you know as it took me a while to even see where and why the CI was still failing. Nothing to do from your side, I will add a fix #1573.

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.

5 participants