Update the Autobahn test suite to v0.8.2 #5679
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
This comment has been minimized.
This comment has been minimized.
| @@ -0,0 +1,15 @@ | |||
| version: "3.9" | |||
There was a problem hiding this comment.
I wonder what would it take to integrate this into pytest
There was a problem hiding this comment.
@anesabml add a link to the new PR here, once you have it. This is useful for interlinking related changes.
There was a problem hiding this comment.
Oh, I thought you'd send it against upstream.
There was a problem hiding this comment.
Here's the new PR against upstream #5809
|
@anesabml did you remove your previous comment? If you can't get this into pytest, we could postpone doing it and you could just try adding separate jobs in the CI maybe calling the same things directly. |
|
@webknjaz integrating with pytest is complicated because pytest-docker-compose usage shines when you have a container that you want to run the test against which is the case when we are testing the client, but for testing the server it's the opposite, which is why I couldn't figure out how to do it yet, which got me thinking if we could drop pytest-docker-compose and use GitHub Actions instead. |
|
You could always just use subprocess invocations and orchestrate spinning the containers from tests (maybe your own fixtures encapsulating this) |
|
For now, please address/comment on my comments, even w/o a CI, and let's move any further integrations to a separate PR. |
|
@webknjaz Should I create an issue first or just open a PR for the integration into CI? |
|
It doesn't really matter, a PR should be enough. |
|
Looks like you marked the comments as resolved but didn't actually make any changes related to them. |
|
OK, I will open a PR. |
|
What comments did I miss? I think I only missed the one about the changes file. |
I usually expect that the PR author opens each collapsed comment in the files tab to check them. Because this is exactly what I'd do otherwise and it just wastes time because you'll need to look at them anyway but after I find time to go through this. This essentially increases the global time to merge for no good reason. |
| @@ -0,0 +1,15 @@ | |||
| version: "3.9" | |||
There was a problem hiding this comment.
@anesabml add a link to the new PR here, once you have it. This is useful for interlinking related changes.
|
@webknjaz I want to open the new PR concerning automating autobahn tests and stack it on top of this PR, however, I don't know how to do so because the autobahn branch isn't present in this repository. |
|
It's in your fork, right? |
|
OK, I will open a PR against my fork and change the base branch after this PR gets merged. |
|
I think you could finish this PR and I'd merge it first |
|
Is there anything you want me to add to this PR? one more thing, can you please check my responses to the comments above? |
|
@anesabml at least, there are merge conflicts that need to be addressed to unblock me. |
|
@webknjaz Should we add a function to extract the results from the generated reports, such as showing how many tests passed and failed and some info about the failing tests? |
|
Maybe. But first go through those comments. I need replies. |
|
@webknjaz I am working on building and pushing a docker image to GitHub container registry instead of building it while running the tests, I have created a workflow but it's failing, so this is currently a WIP. |
|
Maybe submit that separately then? |
|
@webknjaz This was suggested in the original PR, this why I want to include It in this PR. |
|
But if the code will rely on the container that is not published, it'll keep failing and it's impossible to test it without the container before merging. So the container building and publishing should be merged first. |
|
Yes, I totally missed that. I'll remove the changes. |
|
Also, looking at the image, it'll end up being useless in the current form because it will always embed the old version of aiohttp while it's required to test the current one. It'd take a separate effort to redesign this, embedding only the runtime and build deps, and maybe making possible to use pre-built wheels instead of compiling aiohttp on install. |
|
Yes, I have thought about that. Do you have any idea how to approach this problem? |
|
I think it should be a separate PR because this one has too many changes and discussions. And regarding how to approach the problem — you'd need an image that only has the runtime deps and maybe build-time ones too. Then, in CI you'd invoke that container with the source dir and dists mounted as a volume + run |
Backport to 3.8: 💚 backport PR created✅ Backport PR branch: Backported as #5807 🤖 @patchback |
Co-authored-by: Dmitry Erlikh <[email protected]> Co-authored-by: Sviatoslav Sydorenko <[email protected]> (cherry picked from commit fd2ea56)
…0.8.2 (#5807) * Update the Autobahn test suite to v0.8.2 (#5679) Co-authored-by: Dmitry Erlikh <[email protected]> Co-authored-by: Sviatoslav Sydorenko <[email protected]> (cherry picked from commit fd2ea56) * Drop unnecessary attrs install Co-authored-by: Anes Abismail <[email protected]> Co-authored-by: Sviatoslav Sydorenko <[email protected]>
|
Thank you for the suggestion, I will try to work on this when I finish the integration of autobahn tests with pytest. |
What do these changes do?
This patch updates the Autobahn test suite to v0.8.2.
Are there changes in behavior for the user?
No
Related issue number
#4247
Also, this PR incorporates and resolves #5368 by @derlih.
Checklist
CONTRIBUTORS.txtCHANGESfolder<issue_id>.<type>for example (588.bugfix)issue_idchange it to the pr id after creating the pr.feature: Signifying a new feature..bugfix: Signifying a bug fix..doc: Signifying a documentation improvement..removal: Signifying a deprecation or removal of public API..misc: A ticket has been closed, but it is not of interest to users.