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

fileinstall: remove asyncio logic #276

Merged
merged 4 commits into from
Feb 23, 2024
Merged

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Dec 6, 2023

This is one of a triplet of PRs which closes #274



See the cylc-flow PR for testing instructions.

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md no - unreleased bug
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@oliver-sanders oliver-sanders added this to the 1.4.0 milestone Dec 6, 2023
@oliver-sanders oliver-sanders added the bug Something isn't working label Dec 11, 2023
@oliver-sanders oliver-sanders force-pushed the cylc-rose-274 branch 2 times, most recently from 2882d26 to 94b5a2f Compare January 26, 2024 10:31
@oliver-sanders oliver-sanders marked this pull request as ready for review January 26, 2024 10:56
@oliver-sanders oliver-sanders requested a review from wxtim January 26, 2024 10:56
@oliver-sanders oliver-sanders self-assigned this Jan 26, 2024
@oliver-sanders
Copy link
Member Author

Note: this one needs to be merged before this can be tested: #292

@wxtim wxtim mentioned this pull request Feb 5, 2024
6 tasks
@MetRonnie
Copy link
Member

MetRonnie commented Feb 5, 2024

Kicking tests Oh wait, still need cylc/cylc-flow#5868 and metomi/rose#2744

@MetRonnie MetRonnie closed this Feb 5, 2024
@MetRonnie MetRonnie reopened this Feb 5, 2024
* Rose will now take over the role of managing its event loop.
* Addresses cylc#274
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

  • Read changes
  • Run tests locally
  • Tried to break various components of this

@oliver-sanders oliver-sanders mentioned this pull request Feb 22, 2024
8 tasks
@MetRonnie
Copy link
Member

Now kicking tests

@MetRonnie MetRonnie closed this Feb 22, 2024
@MetRonnie MetRonnie reopened this Feb 22, 2024
@oliver-sanders
Copy link
Member Author

:(

@MetRonnie
Copy link
Member

Strange that it's passing locally. Does it want something like this?

diff --git a/tests/functional/test_rose_stem.py b/tests/functional/test_rose_stem.py
index 0e23a73..ebf15f7 100644
--- a/tests/functional/test_rose_stem.py
+++ b/tests/functional/test_rose_stem.py
@@ -373,14 +373,14 @@ class TestProjectOverride():
             "SOURCE_FOO_MIRROR=\"fcm:foo.xm/trunk@1\"",
         ]
     )
-    def test_project_override(self, project_override, expected):
+    async def test_project_override(self, project_override, expected):
         """Check that assorted variables have been exported.
         """
         if expected == 'run_ok':
             assert project_override['run_stem'].returncode == 0
         else:
             expected = expected.format(
-                workingcopy=project_override['workingcopy'],
+                workingcopy=(await project_override)['workingcopy'],
                 hostname=HOST
             )
             assert expected in project_override['jobout_content']

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Feb 22, 2024

This ticket is the gift that just keeps giving 🤦

Shouldn't need to await a fixture, doesn't make sense.

It doesn't look like pytest-asyncio is actually installed?!

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Feb 22, 2024

I'm not convinced these tests were being run properly before.

Note pytest-asyncio wasn't installed in CI (it is for us locally because of cylc-flow).

I've now added pytest-asyncio back in as a test dependency and addressed some unrelated fixture scoping issues that were causing some failures.

* The Cylc install and reinstall interfaces are now async.
* This adapts rose-stem to handle the change and adjusts the
  tests.
yield rose_stem_run_template(rose_stem_opts)


class TestBasic():
Copy link
Member Author

Choose a reason for hiding this comment

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

These test classes aren't achieving anything since they each contain only one test and don't manage any resources.

Test classes date back to unittest, fixtures are more modern solution. Working with module-scoped fixtures and class-scoped tests gets tricky.

Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

All's well that ends well

@oliver-sanders
Copy link
Member Author

@wxtim, you might want to take a quick look at the test changes since you last reviewed. I've added pytest-asyncio as a test dependency (I have no idea what the tests were doing before?) and dealt with some fixture scoping issues that required some test changes.

Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

Re-reviewed! 😄

@wxtim wxtim merged commit f73144c into cylc:master Feb 23, 2024
8 of 9 checks passed
@oliver-sanders oliver-sanders deleted the cylc-rose-274 branch February 23, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reinstall + fileinstall: async clash
3 participants