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

Make asynchronous process test run asynchronously #657

Closed
wants to merge 2 commits into from
Closed

Conversation

huard
Copy link
Collaborator

@huard huard commented May 11, 2022

Overview

The test in test_async was not running asynchronously. This PR runs the test with processing mode set to distributed, and confirms the process completes.

Related Issue / Discussion

Fix #656

Additional Information

I moved the Sleep process definition with the other test processes.

I had trouble initially because I thought owslib would be able to read the status file. Unfortunately, owslib does not support status files on the local file system.

Contribution Agreement

(as per https://github.com/geopython/pywps/blob/master/CONTRIBUTING.rst#contributions-and-licensing)

  • I'd like to contribute [feature X|bugfix Y|docs|something else] to PyWPS. I confirm that my contributions to PyWPS will be compatible with the PyWPS license guidelines at the time of contribution.
  • I have already previously agreed to the PyWPS Contributions and Licensing Guidelines

@huard
Copy link
Collaborator Author

huard commented May 11, 2022

test_async interferes with test_dblog. I solved this locally by shortening the sleep period, but that doesn't seem to be enough here. There should be a cleaner solution to isolate test_dblog.

@huard
Copy link
Collaborator Author

huard commented May 11, 2022

The update_status calls from the async process do not seem to be captured by the logging database in the test suite.
This is explained here: https://www.sqlite.org/inmemorydb.html

Of course, all database connections sharing the in-memory database need to be in the same process

Ideas ?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 81.901% when pulling cbd1c29 on fix-656 into 7839f52 on main.

@huard
Copy link
Collaborator Author

huard commented May 11, 2022

I just renamed the async test so its run at the end.

Note that some of the tests modify the CONFIG, and this has effects on the following tests. It would probably be best to make sure all tests that modify the config return it to its original state.

Note also that test_assync_inout is useless at the moment.

@gschwind
Copy link
Collaborator

Hello,

I did a quick code review, imo you should split dca06fb

into:
Refactor test.create_sleep into test.Sleep
Change test_async to actually test asynchronous process.

and to split cbd1c29, with typo fix separated from the rest.

More over having a loop with proper status check instead of sleep(.5) maybe better in dca06fb

@huard huard mentioned this pull request Sep 16, 2022
2 tasks
assert_response_accepted(resp)

# Wait for process to complete. The test will fail otherwise, which confirms the process is asynchronous.
time.sleep(.5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The check should be done explicitly to validate that the current status is still running with a new checkStatus, but is not yet ProcessSucceeded.

If the process is somehow running in sync, this would not confirm that async was applied. It would just do a small wait before checking the status of an already completed process.

@huard
Copy link
Collaborator Author

huard commented Sep 19, 2022

Replaced by #663 and #664

@huard huard closed this Sep 19, 2022
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.

test_async does not run in async mode
5 participants