-
Notifications
You must be signed in to change notification settings - Fork 110
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
Add lsf unit tests and fix lsf integration test #6739
Conversation
1505e7a
to
dfa61ff
Compare
dfa61ff
to
e5017ce
Compare
e5017ce
to
96b115c
Compare
Is this rebased already? |
assert not Path("bjobs_logs").exists() | ||
|
||
output = caplog.text | ||
assert output == "" |
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.
This line is perhaps a little fragile, as it does not assert our wanted behaviour, only what we log, which we might want to change for other reasons later?
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.
Right, as long as it does not crash it should be fine
96b115c
to
c1bff0e
Compare
asyncio.create_task(self.jobqueue.driver.submit(self)) | ||
task = asyncio.create_task(self._submit_async()) | ||
self.background_tasks.add(task) | ||
task.add_done_callback(self.background_tasks.discard) |
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.
How do we treat exceptions when the task fails?
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.
The callback calls self.somethingwentwrong()
and the exception is re-raised. But it seems like the re-raised exception disappears somewhere in the statemachine.
c1bff0e
to
151446e
Compare
I guess this goes back to backlog |
Closing this as it is no longer relevant. |
Depends on #6643
Resolves #6721