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

Test for ocrd-network #1184

Merged
merged 27 commits into from
May 3, 2024
Merged

Test for ocrd-network #1184

merged 27 commits into from
May 3, 2024

Conversation

joschrew
Copy link
Contributor

@joschrew joschrew commented Feb 8, 2024

This PR adds a test for ocrd-network. Purpose is that the test is available in the docker-image and can then be used with ocrd_all. It does not run with just core itself ( I am not sure if it is good to do it this way, because it is not supposed to run with "just" plain core).

This PR is required for: OCR-D/ocrd_all#407

Copy link
Collaborator

@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.

I don't see a problem integrating this into core proper.

Even the workflow file should be here (besides dummy-workflow.txt), not in ocrd_all.

And core/tests/network/docker-compose.yml could already bind-mount it to the right location. (This compose file in turn can be included in ocrd_all's compose file.)

Since that workflow test cannot be run without ocrd_all, it should simply be skipped by default.

@kba
Copy link
Member

kba commented Mar 13, 2024

I don't see a problem integrating this into core proper.

Even the workflow file should be here (besides dummy-workflow.txt), not in ocrd_all.

And core/tests/network/docker-compose.yml could already bind-mount it to the right location. (This compose file in turn can be included in ocrd_all's compose file.)

Since that workflow test cannot be run without ocrd_all, it should simply be skipped by default.

I see the benefit of DRY and composing the behavior from existing/proven docker-compose.yml. However, I think in this case I think it would be better to not depend on OCR-D/core for the intergration test:

  • it requires some hard-to-read constructs, like build: !reset null and inconsistent relative paths in the derived docker-compose.yml
  • Changing the workflow to run requires updating core
  • The reusable parts, i.e. up RabbitMQ, MongoDB and processing server is generic enough that the benefit of a simple setup (one directory in ocrd_all with the workflow(s), docker-compose.yml and pytest scripts) outweighs the drawback of having some redundancy.

@bertsky
Copy link
Collaborator

bertsky commented Mar 13, 2024

  • Changing the workflow to run requires updating core

ok, good point!

  • The reusable parts, i.e. up RabbitMQ, MongoDB and processing server is generic enough that the benefit of a simple setup (one directory in ocrd_all with the workflow(s), docker-compose.yml and pytest scripts) outweighs the drawback of having some redundancy.

there's a lot of stuff in there, which will still likely evolve a lot. This would have to be synced with ocrd_all then.

If we don't want to inherit/include from core, perhaps we could just have a makefile rule cp from core, and have a few git-controlled override files in place (like docker-compose.override.yml)?

@joschrew joschrew marked this pull request as ready for review March 15, 2024 10:04
@joschrew joschrew requested a review from kba March 22, 2024 10:02
WORKDIR /build-ocrd
COPY Makefile .
RUN make assets
RUN if test -z "$SKIP_ASSETS" || test $SKIP_ASSETS -eq 0 ; then make assets ; fi
Copy link
Member

Choose a reason for hiding this comment

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

I would expect SKIP_ASSETS=0 to disable the behavior. Did you mean test $SKIP_ASSETS -eq 1?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think SKIP_ASSETS should be renamed to MAKE_ASSETS to reverse the logic.

SKIP_ASSETS itself has a negative meaning. False to False makes True.

SKIP_ASSETS=1 (true) -> do not make assets
SKIP_ASSETS=0 (false) -> make assets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for me 0 is false so if SKIP_ASSETS is 0 then it is not skipped.

tests/network/test_ocrd_all_workflow.py Outdated Show resolved Hide resolved
@kba kba force-pushed the test-workflow branch 4 times, most recently from 12df7c8 to 0afb6a1 Compare April 16, 2024 14:18
@kba kba merged commit 1bd8fc4 into master May 3, 2024
21 checks passed
@kba kba deleted the test-workflow branch May 3, 2024 16:56
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.

4 participants