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

Automating e2e tests #769

Merged

Conversation

vcastellm
Copy link
Contributor

Description

This is the first of a series of PRs to bring e2e testing to Edge, in this PR we enable automation of running e2e tests with GH actions.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Documentation update

Updated e2e/README.md

Additional comments

One e2e test is failing, now detected because automation, but fixing the test itself is out of the scope of this PR.

@vcastellm
Copy link
Contributor Author

Additional comment: It looks like there's a flaky test in e2e
image

@codecov
Copy link

codecov bot commented Sep 28, 2022

Codecov Report

Merging #769 (c242c09) into develop (49b9e68) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #769   +/-   ##
========================================
  Coverage    52.71%   52.72%           
========================================
  Files          130      130           
  Lines        17147    17147           
========================================
+ Hits          9039     9040    +1     
  Misses        7460     7460           
+ Partials       648      647    -1     
Impacted Files Coverage Δ
network/server.go 75.75% <ø> (-0.43%) ⬇️
network/server_discovery.go 75.79% <0.00%> (+1.91%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal left a comment

Choose a reason for hiding this comment

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

Generally looks good, but posting comments with observations.

.github/workflows/e2e.yaml Show resolved Hide resolved
e2e/README.md Outdated Show resolved Hide resolved
.github/workflows/e2e.yaml Show resolved Hide resolved
Copy link
Contributor

@ferranbt ferranbt left a comment

Choose a reason for hiding this comment

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

LGTM!

@vcastellm vcastellm mentioned this pull request Sep 28, 2022
11 tasks
Copy link
Contributor

@Kourin1996 Kourin1996 left a comment

Choose a reason for hiding this comment

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

Thank you for PR! I've left only suggestions. Please check and change if you feel need. Thank you

Makefile Outdated Show resolved Hide resolved
@Stefan-Ethernal
Copy link
Collaborator

Stefan-Ethernal commented Sep 29, 2022

Also just another general feedback on e2e framework refactoring, which we can decide later on...

On v3, we have introduced abstractions for cluster and bridge and I'd say that looks better than implementing all those functions, which don't belong to the TestServer itself in the helper.go (test-cluster.go, test-bridge.go etc.)

Copy link
Contributor

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Looks good 💯

@vcastellm
Copy link
Contributor Author

@Stefan-Ethernal on that observation, the corresponding abstraction to test-cluster here would be https://github.com/0xPolygon/polygon-edge/blob/develop/e2e/framework/ibft.go#L19 and I can move the resolve function up there and pass it as config to the test server, as I did in the #771, I'd say, let's go with this now, and I would propose this refactor in that PR to see how it looks.

Copy link
Contributor

@ZeljkoBenovic ZeljkoBenovic left a comment

Choose a reason for hiding this comment

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

Great job. 🥇
Left a minor comment.

Makefile Outdated Show resolved Hide resolved
@vcastellm vcastellm merged commit ae056ef into develop Sep 30, 2022
@vcastellm vcastellm deleted the feature/EVM-45-migrate-e-2-e-consensus-tests-to-edge branch September 30, 2022 15:09
@github-actions github-actions bot locked and limited conversation to collaborators Sep 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants