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

Add make test-kitodo #63

Merged
merged 18 commits into from
Mar 14, 2023
Merged

Add make test-kitodo #63

merged 18 commits into from
Mar 14, 2023

Conversation

markusweigelt
Copy link
Collaborator

No description provided.

add kitodo test
maybe not necessary cause when we catch the error the tmate session is already closed
@markusweigelt
Copy link
Collaborator Author

Atm we call make test and run all test. Possibly only test-kitodo and test-presentation is still necessary as the test-production is indirectly tested by test-kitodo.

Copy link
Member

@bertsky bertsky left a comment

Choose a reason for hiding this comment

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

Excellent!

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated
# run asynchronous ocr processing, which should return within 5 seconds with exit status 1
timeout --preserve-status 5 docker exec -t `docker container ls -qf name=kitodo-app` bash -c '/usr/local/kitodo/scripts/script_ocr_process_dir.sh "testdata-kitodo" 1'; test $$? -eq 1
# Check the docker log of the manager if the ocr processing has already been completed. It fails if the ocr processing was not completed within 5 minutes.
( timeout 5m docker logs -f `docker container ls -qf name=ocrd-manager` & ) | grep -q "KitodoActiveMQClient"
Copy link
Member

Choose a reason for hiding this comment

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

Very nice recipe!

Makefile Outdated Show resolved Hide resolved
@bertsky
Copy link
Member

bertsky commented Mar 10, 2023

Possibly only test-kitodo and test-presentation is still necessary as the test-production is indirectly tested by test-kitodo.

Yes, but test-production (alone) tests in sync mode, which is good for coverage.

BTW, on the CI we could do make -j test which will run all 3 tests in parallel. To make that work with the Controller, we can either add CONTROLLER_WORKERS=3 make -j test so that they will actually be computed in parallel, or keep the default 1 (which will have each job wait for the previous job to free the semaphore). We could even test both...

markusweigelt and others added 6 commits March 10, 2023 11:25
Co-authored-by: Robert Sachunsky <[email protected]>
Co-authored-by: Robert Sachunsky <[email protected]>
Co-authored-by: Robert Sachunsky <[email protected]>
Co-authored-by: Robert Sachunsky <[email protected]>
add APP_DATA
@markusweigelt
Copy link
Collaborator Author

markusweigelt commented Mar 10, 2023

BTW, on the CI we could do make -j test which will run all 3 tests in parallel. To make that work with the Controller, we can either add CONTROLLER_WORKERS=3 make -j test so that they will actually be computed in parallel, or keep the default 1 (which will have each job wait for the previous job to free the semaphore). We could even test both...

Probably we have to adjust the make call in Makefile too

$(MAKE) -C _modules/ocrd_manager $@
.

Atm rerunning of test-kitodo without make down and make start is a problem cause we need to flush the manager log to provide the check for KitodoActiveMQClient if ocr processing has finished.

( timeout 5m docker logs -f `docker container ls -qf name=ocrd-manager` & ) | grep -q "KitodoActiveMQClient"

truncate -s 0 $(docker inspect --format='{{.LogPath}}' ocrd_kitodo-ocrd-manager-1 ) flush's the log with sudo but probably there are permission issues without sudo at the runner. But i will check this.

@bertsky
Copy link
Member

bertsky commented Mar 10, 2023

Atm rerunning of test-kitodo without make down and make start is a problem cause we need to flush the manager log to provide the check for KitodoActiveMQClient if ocr processing has finished.

indeed.

But truncating the Docker log file is asking for trouble. I'd rather keep it simple and require down-up.

Or is there another way to query the ActiveMQ was received, perhaps something on the process directory?

@markusweigelt
Copy link
Collaborator Author

Yes. Instead of testing for existing KitodoActiveMQClient request in log, we can check if the alto files are already in the directory. If they are not present within of 5 minutes, an error will be thrown. We would only have to delete the alto directory before rerun next test.

@bertsky
Copy link
Member

bertsky commented Mar 10, 2023

Yes, sounds good. (Or directly intercept the ActiveMQ result by interacting with our kitodo-mq container?)

@markusweigelt
Copy link
Collaborator Author

markusweigelt commented Mar 10, 2023

Changed the implementation to check for existing of an alto file.

Remove pull_request event trigger cause workflow runs two times.
Copy link
Member

@bertsky bertsky left a comment

Choose a reason for hiding this comment

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

But I really like the ActiveMQ test better!

(Even if that means you cannot easily repeat. And why not try to test directly for the ActiveMQ reply? For example, we could do docker exec kitodo-mq bin/activemq consumer --messageCount 1 ...)

Makefile Outdated Show resolved Hide resolved
Co-authored-by: Robert Sachunsky <[email protected]>
@markusweigelt markusweigelt merged commit c5b5be8 into main Mar 14, 2023
@markusweigelt markusweigelt deleted the test-kitodo branch March 14, 2023 11:47
@markusweigelt
Copy link
Collaborator Author

Add the issue #64 and merge branch as discussed

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