-
Notifications
You must be signed in to change notification settings - Fork 442
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 unit test for each queue jobs class #9899
Comments
…obs\doi\DepositContext
…d PKP\jobs\bulk\BulkEmailSender along with mockery package version update
…omSearchIndexJob and PKP\jobs\submissions\RemoveSubmissionFileFromSearchIndexJob
…obs\doi\DepositContext
…d PKP\jobs\bulk\BulkEmailSender along with mockery package version update
…omSearchIndexJob and PKP\jobs\submissions\RemoveSubmissionFileFromSearchIndexJob
@asmecher can you take a look the PR at #9899 (comment) for the OJS only ? |
@withanage can you please take a look at this particular implementation to mock the implementation of mock at PKPTestCase and to facilitate it , I had to add few logic within the core app at PKPApplication and PKPContainer. Long story short, we like to mock the guzzle requests without making any actual request but our very static like We can also decide not to mock it for 3.4.0 and have something better like Also any suggestion or advise on approach on the job test implementation are welcomed . |
PKPTestCaseI have looked at your test. I also think that the way you have implmented, it is the best possible way to mock the Guzzle HTTP client and I can't offer a better approach either. General support
|
@touhidurabir, for some reason your .php additions in https://github.com/pkp/ojs/pull/4285/files are being flagged as binary by Github; is there some unusual encoding or something causing this? |
^ I suspect it's the |
@asmecher I just noticed the issue in github as it is not causing any issue for me in my editor(VS code or sublime) . I think we can easily solve it using the base64 encode/decode. At least anyone one can see the test file but wont be able to understand the what props are defined there with a single look as base64 encoded string . I think it's worth solving and will only take about an hour or two to update all the tests . for me it's not causing any issue in my editor but may cause issue with different editor for other . |
…le encoding issue
…inary file encoding issue
@asmecher I have updated the tests to resolve the binary file issue . Good to take a look now . |
Describe the problem
Starting from
3.4.0
we have queue jobs to handle long running tasks or for tasks that need to done later . Our queue jobs implementation is an extended implementation of Laravel queue jobs.The problem arise or will arise when any of the jobs structure will update but there is one or more pending jobs with older structure already dispatched in queue waiting to get executed . This problem get more complex with the upgrade of newer version when older structure based job still in queue as they may continue to fail and no way to resolve it after upgrade which may lead to undesirable outcome.
Describe the proposal
The initial plan was to force to run all pending queue jobs before upgrade which proposed at Add pre-flight check for pending jobs before running upgrade . This was a very simple and easy approach but may not suited for all possible cases . Also a job class structure or handle implementation may need to update based on requirement.
After some detailed discussion, we decided to add unit tests for each job class to make sure the consistence update the job class structure and handle method implementation . The idea is to try to mock each of jobs class to make sure it's on even after any update to it.
Impacted version
OJS, OMP or OPS version 3.4.0 and main
Additional information
Details discussion at Right approach to handle pending queue jobs at upgrade
Job classes to have unit tests
Stable-3.4.0
OJS
OMP
OPS
PKP-LIB
PRs
pkp-lib --> #9981
ojs --> pkp/ojs#4285
The text was updated successfully, but these errors were encountered: