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

Improve the mise script #170

Merged
merged 11 commits into from
Jan 26, 2025
Merged

Improve the mise script #170

merged 11 commits into from
Jan 26, 2025

Conversation

syntrust
Copy link

@syntrust syntrust commented Jan 21, 2025

Add semgrep tests for code static analysis. These tests were missing in local tests compared to CircleCI's main workflow.

Additionally, put the lint check early before solidity building. This step helps identify common format errors and can be completed quickly.

Other changes:

  • environment check like RPC and kurtosis
  • remove duplicated e2e tests; already included in go tests
  • time metering

@blockchaindevsh
Copy link
Collaborator

blockchaindevsh commented Jan 21, 2025

It's helpful to add an estimation about the time to finish running it locally each time more tasks are added to it.

@syntrust
Copy link
Author

It's helpful to add an estimation about the time to finish running it locally when more tasks are added to it.

Print time duration in the end.

@qzhodl
Copy link

qzhodl commented Jan 22, 2025

It's helpful to add an estimation about the time to finish running it locally when more tasks are added to it.

Print time duration in the end.

I guess what @zhiqiangxu meant is to include a comment specifying the approximate time each added task will take to complete on a local machine?

@blockchaindevsh
Copy link
Collaborator

Print time duration in the end.

I mean adding a description in the PR about the incremented time and total time to run the task on local machine, we'll have to keep an eye on it to avoid it suddenly takes too much time.

@syntrust
Copy link
Author

It's helpful to add an estimation about the time to finish running it locally when more tasks are added to it.

Print time duration in the end.

Print time duration in the end.

I mean adding a description in the PR about the incremented time and total time to run the task on local machine, we'll have to keep an eye on it to avoid it suddenly takes too much time.

It makes more sense to check the real execution time locally in person due to different hardware conditions.
It is recommended to use the script in person, and printing time in the script helps to get a feel of the duration for each step after you run it a few times.

@blockchaindevsh
Copy link
Collaborator

It's helpful to add an estimation about the time to finish running it locally when more tasks are added to it.

Print time duration in the end.

Print time duration in the end.

I mean adding a description in the PR about the incremented time and total time to run the task on local machine, we'll have to keep an eye on it to avoid it suddenly takes too much time.

It makes more sense to check the real execution time locally in person due to different hardware conditions. It is recommended to use the script in person, and printing time in the script helps to get a feel of the duration for each step after you run it a few times.

It doesn't need to be accurate, just a baseline we have in mind. If the reviewee doesn't provide such numbers, all other reviewers will need to do it, which means duplicated work is done multiple times.

@syntrust syntrust changed the title Add semgrep tests to mise script Improve the mise script Jan 25, 2025
@syntrust
Copy link
Author

It's helpful to add an estimation about the time to finish running it locally when more tasks are added to it.

Print time duration in the end.

Print time duration in the end.

I mean adding a description in the PR about the incremented time and total time to run the task on local machine, we'll have to keep an eye on it to avoid it suddenly takes too much time.

It makes more sense to check the real execution time locally in person due to different hardware conditions. It is recommended to use the script in person, and printing time in the script helps to get a feel of the duration for each step after you run it a few times.

It doesn't need to be accurate, just a baseline we have in mind. If the reviewee doesn't provide such numbers, all other reviewers will need to do it, which means duplicated work is done multiple times.

The following newly added checks take less than 5 seconds each:

  • just semgrep
  • just semgrep-test
  • just lint-check

The full tests run about 18 minutes on AX101, and take approximately 11 hours on my local MacBook Pro.
Hope this helps.

@syntrust syntrust mentioned this pull request Jan 26, 2025
Copy link

@qzhodl qzhodl left a comment

Choose a reason for hiding this comment

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

LGTM

@dajuguan
Copy link

The mise script runs successfully on my machine.

@syntrust syntrust merged commit ce75b7c into op-es Jan 26, 2025
@syntrust syntrust deleted the dl-mise-1 branch January 26, 2025 10:08
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.

4 participants