Skip to content

chore(tooling): bump ruff and questionary and fix vsc ruff warning and remove non-existent rule SC200 #1525

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

felix314159
Copy link
Collaborator

🗒️ Description

I think the warning WARN ruff_server::server::api Received notification $/setTrace which does not have a handler. is the reason we kept seeing ruff warnings at every vsc startup, this can be ignored so setting the warn level from warn to error resolves this annoyance. I also bumped ruff to the latest version and did the necessary fixes (SC200 rule does not exist), this was spotted via uv run ruff check --no-fix --show-fixes src tests docs .github/scripts and manually resolved. Do we even use questionary package? I just saw the comment and bumped it to newer version.

🔗 Related Issues

✅ Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
  • Tests: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

@felix314159
Copy link
Collaborator Author

felix314159 commented May 2, 2025

To clarify commit e6d6336: running uv run pytest -c ./pytest-framework.ini fails a few solc related tests on ARM. It ends up trying to use some x86 solc, these adjustments aim to ensure that the same solc binary is used that is used everywhere else. If you only want to run a subset of tests (e.g. only solc test) use this: uv run pytest src/pytest_plugins/solc/tests/test_solc.py -c ./pytest-framework.ini

@marioevz
Copy link
Member

marioevz commented May 2, 2025

To clarify commit e6d6336: running uv run pytest -c ./pytest-framework.ini fails a few solc related tests on ARM. It ends up trying to use some x86 solc, these adjustments aim to ensure that the same solc binary is used that is used everywhere else. If you only want to run a subset of tests (e.g. only solc test) use this: uv run pytest src/pytest_plugins/solc/tests/test_solc.py -c ./pytest-framework.ini

I still don't quite understand this commit, are changes in src/pytest_plugins/solc/tests/test_solc.py meant to be included or were they debugging changes?

Locally, the test fails for me in arm with e6d6336f93184ad4b05f2289066eb45e92e5038c.

cc @danceratopz @spencer-tb I think you are more familiar with these tests than I am.

@felix314159
Copy link
Collaborator Author

To clarify commit e6d6336: running uv run pytest -c ./pytest-framework.ini fails a few solc related tests on ARM. It ends up trying to use some x86 solc, these adjustments aim to ensure that the same solc binary is used that is used everywhere else. If you only want to run a subset of tests (e.g. only solc test) use this: uv run pytest src/pytest_plugins/solc/tests/test_solc.py -c ./pytest-framework.ini

I still don't quite understand this commit, are changes in src/pytest_plugins/solc/tests/test_solc.py meant to be included or were they debugging changes?

Locally, the test fails for me in arm with e6d6336f93184ad4b05f2289066eb45e92e5038c.

cc @danceratopz @spencer-tb I think you are more familiar with these tests than I am.

It worked on my machine but it is not a robust way of doing things so let's not merge this for now if it does not work for you. I think the root cause of these issues is summarized here and the best solution is to push solidity team to publish arm linux binaries so that solc-select can be patched to start working as expected on every (commonly-used) platform

Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Good work spotting these problems and thanks a lot for tackling them!

There's too many different types of changes in this PR for my taste. I'd prefer if dependency changes occur in their own dedicated PR when possible. If there's a change in behavior, it then becomes easier to track the source down and it can be easily reverted in an understandable way (via git revert`) without undoing other work.

In particular, the unit test changes don't fit within the scope of the other changes.

Could we just apply the dep changes (and if you like, the ruff config changes) in this PR and move the unit test changes to a separate PR?

@felix314159
Copy link
Collaborator Author

All done, moved the yul and solc related stuff out of this PR. Thanks for reviewing!

@felix314159 felix314159 requested a review from danceratopz May 7, 2025 08:58
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.

3 participants