Update repo-config from v0.11.0 to v0.13.0#117
Conversation
ghost
self-requested a review
as a 
github-actions
Bot
added
the
part:tooling
There was a problem hiding this comment.
Pull Request Overview
This PR updates repo-config from v0.11.0 to v0.13.0 and upgrades several GitHub Actions as well as the Ubuntu runner version to 24.04. Key changes include:
- Upgrading actions (e.g., setup-git, setup-python-with-deps, gh-action-nox) to v1.0.0.
- Modifying job matrices to include an additional architecture and updating the runs-on values.
- Adjusting Dependabot grouping rules for dependency updates.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| .github/workflows/ci.yaml | Upgraded action versions, updated runner versions, and modified matrix logic. |
| .github/workflows/ci-pr.yaml | Introduced new PR-specific CI with updated actions and configuration. |
| .github/dependabot.yml | Modified dependency grouping, especially for repo-config and minor updates. |
Comments suppressed due to low confidence (3)
.github/workflows/ci.yaml:44
- [nitpick] Consider adding a brief comment above this line to explain the conditional formatting for runs-on; this will improve clarity for future maintainers.
runs-on: ${{ matrix.os }}${{ matrix.arch != 'amd64' && format('-{0}', matrix.arch) || '' }}
.github/workflows/ci.yaml:87
- Ensure that the new setup-python action handles pip caching or any required dependency caching similarly to the previous version, since the old configuration included a pip cache parameter.
uses: frequenz-floss/gh-action-setup-python-with-deps@v1.0.0
.github/dependabot.yml:35
- The minor update group excludes 'frequenz-repo-config*', yet a dedicated group for repo-config updates is defined later. Confirm that this exclusion is intentional in order to avoid duplicate PRs for repo-config dependency updates.
exclude-patterns:
- "async-solipsism"
- "frequenz-repo-config*"
- "markdown-callouts"
- "mkdocs-gen-files"
- "mkdocs-literate-nav"
- "mkdocstrings*"
- "pydoclint"
- "pytest-asyncio"
Signed-off-by: camille-bouvy-frequenz <camille.bouvy@frequenz.com>
Signed-off-by: camille-bouvy-frequenz <camille.bouvy@frequenz.com>
Signed-off-by: camille-bouvy-frequenz <camille.bouvy@frequenz.com>
|
Hey @shsms, I recon adding the following in now breaks the |
|
It should be fine to create an issue in the base client and then exclude the UnraiseableException from the list. |
|
I just had a look at the failing tests and damn, things got quite complicated in those tests. Is there any reason why you are manually creating the async loop instead of using pytest_asyncio and configure it to have a function scope? Also the mocking of async iterators seems to be pretty messed up/wrong. Streaming functions are mocked but not the iterator they return. It could definitely take some time to fix those tests... :-/ |
github-actions
Bot
added
the
part:tests
|
I pushed a commit to ignore warnings in the problematic tests for now, I leave creating the issue to you @shsms |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
matthias-wende-frequenz
removed this pull request from the merge queue due to a manual request
|
OK, tests still fail in the merge queue, it seems to be very flaky and affect most tests. I'm having a second look and think I can fix the warning for real. |
|
OK, first approach didn't work out. There were more issues. I think I have all of them fixed now, it took a bit longer than what I would have wanted, but I hope we can continue with a warning-free project now 🤞 If tests pass I will remove the commit ignoring the warnings and the revert. It was really tricky to find out what was going on because |
|
Test seem to be passing now, will do the final rebasing and activate auto-merge. |
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
This helps detecting issues with cleanup in the place where they happen. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
There is really no need to create our own loop for these tests, we can just use the loop provided by `pytest-asyncio` which already offers a lot of tools for detecting issues and configure how we want the loop to work. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
We are already using the option to auto-use the `pytest-asyncio` plugin for every `async` function, so we don't need to mark them manually. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
By default AsyncMock doesn't provide any proper mocking of async
async iterators, and if we just assign a `AsyncMock` to stub calls like
`ReceivePublicTradesStream`, what happens is that
`async for x in ReceivePublicTradesStream()` is used instead. What
Python does with this is something like:
```py
stream = ReceivePublicTradesStream()
it = stream.__aiter__() # Gets an anync iterator, but this call is sync
while v := await it.__anext():
...
```
Because `ReceivePublicTradesStream` is an `AsyncMock`, getting any
attributes (like `__aiter__()`) results in returning a new `AsyncMock`.
But since `__aiter__()` is called without an `await`, then we get the
following exceptions/warnings:
* TypeError("'async for' requires an object with __aiter__ method, got
coroutine")
* RuntimeWarning: coroutine 'AsyncMockMixin._execute_mock_call' was
never awaited
To fix this, we just crate a proper fake async iterator we can return
to the async mock.
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
There was a problem hiding this comment.
Pull Request Overview
This PR updates the dependency version for frequenz-repo-config from v0.11.0 to v0.13.0 while also modernizing several test modules and CI workflows. Key changes include:
- Updating dependency versions and keywords in pyproject.toml and related configuration files.
- Refactoring tests to use async/await syntax by removing manual event loop management and async marker decorators.
- Revamping CI workflows for improved architecture support, updated GitHub Actions versions, and removal of deprecated macros.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_client.py | Removed manual event loop creation and replaced with async/await in test functions. |
| pyproject.toml | Upgraded frequenz-repo-config version and reformatted keywords. |
| mkdocs.yml | Updated macros configuration to reference the new module and streamlined comments. |
| integration_tests/test_api.py | Removed redundant @pytest.mark.asyncio decorators from async test functions. |
| docs/_scripts/macros.py | Removed the macros file, implying a shift to the new mkdocs macros configuration. |
| RELEASE_NOTES.md | Added a release note line documenting the repo-config upgrade. |
| .github/workflows/ci.yaml | Updated workflow definitions including runs-on changes, architecture matrix, and action versions. |
| .github/workflows/ci-pr.yaml | Introduced a new PR testing workflow using updated actions. |
| .github/dependabot.yml | Adjusted dependency grouping strategies for automated dependency updates. |
Comments suppressed due to low confidence (3)
integration_tests/test_api.py:144
- Ensure that removal of the @pytest.mark.asyncio decorators doesn't impact test execution; verify that your pytest configuration supports async tests natively.
@pytest.mark.asyncio
docs/_scripts/macros.py:1
- Confirm that removing the macros file does not disrupt documentation generation processes; update documentation macros configuration if any dependent references exist.
Entire file removed
.github/workflows/ci.yaml:44
- Check that the conditional expression used in the 'runs-on' field is supported by your GitHub Actions runner; verify that its syntax is parsed correctly for non-amd64 architectures.
runs-on: ${{ matrix.os }}${{ matrix.arch != 'amd64' && format('-{0}', matrix.arch) || '' }}
|
BTW, ready to merge again, it just needs a new approval @matthias-wende-frequenz |
No description provided.