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

Refactor #103

Merged
merged 1 commit into from
Dec 10, 2024
Merged

Refactor #103

merged 1 commit into from
Dec 10, 2024

Conversation

GDYendell
Copy link
Contributor

@marcelldls I have been playing around. What do you think of this?

Seems to be failing on a tango test. Should check if this is broken on main as well.

@GDYendell GDYendell requested a review from marcelldls December 9, 2024 17:26
src/fastcs/transport/graphQL/graphQL.py Outdated Show resolved Hide resolved
src/fastcs/transport/graphQL/graphQL.py Outdated Show resolved Hide resolved
@marcelldls
Copy link
Contributor

@marcelldls I have been playing around. What do you think of this?

Seems to be failing on a tango test. Should check if this is broken on main as well.

Error is: ResourceWarning: unclosed <socket.socket fd=28, family=1, type=1, proto=0>

As far as the testing goes, your tests work on my machine in a devcontainer... Im going to see if I can reproduce this

@marcelldls
Copy link
Contributor

marcelldls commented Dec 10, 2024

I have a branch graphql-backend-g-m which also fails. However, on a whim if I suppress all the epics tests, then all the tango tests pass https://github.com/DiamondLightSource/FastCS/actions/runs/12256441074

This would make me think that we have a tear down problem? It doesnt make sense that the test which fails, fails but the others pass. I feel like pytest is really annoying with its side effects as a first class citizen...

@marcelldls
Copy link
Contributor

I reran the graphql-backend tests which were previously passing - they now also fail https://github.com/DiamondLightSource/FastCS/actions/runs/12139830254/job/34201957331?pr=71

Copy link
Contributor

@marcelldls marcelldls left a comment

Choose a reason for hiding this comment

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

Looks great - thanks for the docstrings

@GDYendell GDYendell merged commit 24d8e22 into graphql-backend Dec 10, 2024
13 of 15 checks passed
@GDYendell GDYendell deleted the graphql-backend-g branch December 10, 2024 15:44
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.

2 participants