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

108 enhancement/add ci and do housekeeping #154

Merged
merged 120 commits into from
Jul 26, 2024

Conversation

Rajil1213
Copy link
Contributor

@Rajil1213 Rajil1213 commented Jul 16, 2024

Description

This PR adds a github-actions CI pipeline with the following workflows:

  • A monthly check for dependency updates
  • A meta workflow to lint the github-actions
  • Linting of the binaries (alpen-vertex-sequencer) and all crates, as well as TOML files
  • cargo check via cargo-hack (similar to reth)
  • cargo fmt based on rustfmt.toml
  • codespell based on .codespellrc
  • cargo audit to check for security vulnerabilities in dependencies
  • cargo nextest along with coverage report via codecov; no coverage threshold has been set except that no PR should decrease the overall coverage
  • cargo mutants that modifies the codebase and re-runs the tests to check if these mutations are caught by the tests; this returns a lot of errors so this has been allowed to fail but future reviews should consider this output
  • doctests (although there are no doctests in the repo)
  • dependabot for updates to github-actions and python packages for functional tests

A PR template has also been added to standardize PR descriptions, and an editor configuration for vscode (and others) has been added so as to avoid having to review superfluous changes brought about by different editor configs.

Checklist

  • I have performed a self-review of my code.
  • I have commented my code where necessary.
  • I have updated the documentation if needed.
  • My changes do not introduce new warnings.
  • I have added tests that prove my changes are effective or that my feature works.
  • New and existing tests pass with my changes.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactor

Related Issues

Closes #108

Closes #80

Additional Context

This PR introduces a lot of changes in the source code to get the lint checks and tests to pass which can potentially complicate merging of any existing PRs.

@Rajil1213 Rajil1213 linked an issue Jul 16, 2024 that may be closed by this pull request
@Rajil1213 Rajil1213 marked this pull request as draft July 16, 2024 08:07
@MdTeach
Copy link
Contributor

MdTeach commented Jul 16, 2024

Also, should we add linting rules for the .toml files ?
Something like Taplo

@Rajil1213 Rajil1213 force-pushed the 108-enhancement/add-ci-and-do-housekeeping branch from 528356d to 773427b Compare July 16, 2024 09:12
@Rajil1213
Copy link
Contributor Author

Also, should we add linting rules for the .toml files ? Something like Taplo

Yes, I think that is a good idea. Will add that once the current suite of tests pass.

Copy link

codecov bot commented Jul 16, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment

Thanks for integrating Codecov - We've got you covered ☂️

@Rajil1213 Rajil1213 force-pushed the 108-enhancement/add-ci-and-do-housekeeping branch from d88abd9 to 8805722 Compare July 16, 2024 12:00
@Rajil1213 Rajil1213 marked this pull request as ready for review July 16, 2024 12:00
@Rajil1213 Rajil1213 force-pushed the 108-enhancement/add-ci-and-do-housekeeping branch from 86d31b8 to 93e1ae2 Compare July 16, 2024 12:30
@Rajil1213 Rajil1213 self-assigned this Jul 16, 2024
Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

Overall looks great. Main comments are related to Clippy and personal subjective things.

.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
.github/workflows/lint.yml Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
crates/consensus-logic/src/message.rs Outdated Show resolved Hide resolved
rustfmt.toml Show resolved Hide resolved
crates/db/src/stubs/l2.rs Show resolved Hide resolved
crates/db/src/chain_state/db.rs Outdated Show resolved Hide resolved
@Rajil1213 Rajil1213 force-pushed the 108-enhancement/add-ci-and-do-housekeeping branch 2 times, most recently from 2b68dfb to 4978fcf Compare July 22, 2024 05:28
@Rajil1213
Copy link
Contributor Author

@delbonis I am trying to integrate the functional tests in the CI (and in this PR). Since the main function in entry.py returns 0 regardless of the outcome of the tests, the CI does not fail even when the tests do. I also cannot see a way to get flexitest to fail (and return a non-zero exit code) once any of the tests fail. Is there something I'm missing here?

@Rajil1213
Copy link
Contributor Author

@delbonis after a brief debugging session with @voidash and @sapinb, we discovered that all the tests depend on the same bitcoin datadir causing them to interfere with each other. So, we've refactored the main function in entry.py to run each test on a separate runtime (env, datadir). Now, the tests are passing. But even so, it is not possible to tell whether the tests are passing/failing without inspecting the CI logs.

@Rajil1213 Rajil1213 requested a review from voidash July 23, 2024 09:04
@Rajil1213 Rajil1213 added the enhancement New feature or request label Jul 23, 2024
@john-light john-light requested a review from delbonis July 23, 2024 13:13
@delbonis
Copy link
Contributor

@Rajil1213 Perhaps I should have documented this more thoroughly. A design goal of Flexitest is that the test environments are able to be reused across tests to reduce startup times and permit them to run in a more realistic environment. If the tests are stepping on each other then they should be adjusted to either (1) not do that or (2) run with a dedicated env by passing an EnvConfig in the call to ctx.set_env on construction. (ie. ctx.set_env(BasicEnvConfig()))

Wrapping the whole setup in a loop kinda defeats the whole purpose of having a pass to consolidate tests that use the same env into the same test run that the test runtime does. There is the bug that if two envs running at the same time may have conflicting service datadir names, which should be fixed, but in the meantime can be sidestepped by having the services have different names.

As for the returning failing exit code, the code that constructs the result records are constructed is here. The status field can be inspected and if it's anything but "ok" then that's a failing condition. I will go and expose a method to wrap that up, though so this can be simplified in the future.

@Rajil1213
Copy link
Contributor Author

@delbonis as per your suggestion, I have refactored the tests a bit to use a different config for the one test that messes up the bitcoin blockchain i.e., the reorg test. I've also changed the datadir construction to use a random string (prefixed with bitcoin_ and seq_) instead of just bitcoin and seq. This seems to have fixed the tests for now.

@delbonis
Copy link
Contributor

Flexitest has been updated to have the functionality needed for this, probably have to refresh the poetry.lock.

@Rajil1213 Rajil1213 force-pushed the 108-enhancement/add-ci-and-do-housekeeping branch from 1f7782b to 9a5600e Compare July 24, 2024 06:44
@Rajil1213
Copy link
Contributor Author

Flexitest has been updated to have the functionality needed for this, probably have to refresh the poetry.lock.

I've refreshed the lockfile and updated the tests. Things seem to be working fine now. 😄

@delbonis
Copy link
Contributor

Also can we squash these commits? This is kinda a lot.

@Rajil1213
Copy link
Contributor Author

Also can we squash these commits? This is kinda a lot.

Yes, I think that should be fine as long as the git blame is still accurate. I've gone through the commits and only very few serve as documentation and none we can't do without.

@Rajil1213 Rajil1213 force-pushed the 108-enhancement/add-ci-and-do-housekeeping branch from 782efa7 to f985f08 Compare July 25, 2024 03:53
@Rajil1213
Copy link
Contributor Author

Rajil1213 commented Jul 25, 2024

@delbonis as part of this PR, I've also renamed the test directory to the more explicit functional-tests. This is to disambiguate with Rust's tests directory which carries a special meaning (having both a test and tests directory might be confusing). I expect that we'll be adding some integration tests in the tests directory before long (this is at least the plan for the bridge rpc service that I'll be working on later).

@Rajil1213 Rajil1213 force-pushed the 108-enhancement/add-ci-and-do-housekeeping branch from 9ff6d2d to 85fd9dd Compare July 25, 2024 09:51
delbonis
delbonis previously approved these changes Jul 25, 2024
Copy link
Contributor

@delbonis delbonis 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, just that one question.

functional-tests/entry.py Outdated Show resolved Hide resolved
@Rajil1213 Rajil1213 merged commit 1191dc4 into master Jul 26, 2024
21 of 23 checks passed
@Rajil1213 Rajil1213 deleted the 108-enhancement/add-ci-and-do-housekeeping branch July 30, 2024 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CI and do some housekeeping Clean up unused imports
3 participants