-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
feat: +unit test #663
feat: +unit test #663
Conversation
83eb54b
to
d9c5809
Compare
tests/metagpt/utils/test_s3.py
Outdated
@pytest.mark.asyncio | ||
async def test_s3_no_error(): | ||
key = CONFIG.S3_ACCESS_KEY | ||
CONFIG.S3_ACCESS_KEY = "YOUR_S3_ACCESS_KEY" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a general sense, CONFIG is a read-only class and should not be writable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONFIG
is a session-scoped context that supports both read and write operations.Config
represents a configuration file and is read-only.- The root cause of the issue here is the absence of the concept of a session in the console version. Therefore, the name
CONFIG
is used. - To support the concept of a session functionally, the
contextvars.ContextVar
data type has been employed. - All mock session options have been changed to use the
set_context
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's going to be a bit tricky about that. The associated code needs to be refactored to be more straightforward. If it is a session concept, you should provide a session object, and use session.config or context.config in the session object to accurately express the context in which it is located. It needs to be refactored in the future, so you can put it here first
66c1970
to
b7d74c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are a few legacy issues:
- Avoid using force push, which here seems to mask some historical commits
- Reduce the amount of changes to CONFIG, considering that it must be refactored
Features