Nimh 7 take 2 #25
Nimh 7 take 2 #25
Conversation
tests/test_file_processing.py
Outdated
| @@ -16,7 +17,11 @@ def sample_pdf(tmp_path): | |||
|
|
|||
| def test_pdf_converter(caplog, sample_pdf): | |||
| caplog.set_level(logging.INFO) | |||
| file_id = "test_file" | |||
| output_file = f'docs/examples/sciencebeam_xml_outputs/{file_id}.xml' | |||
There was a problem hiding this comment.
see other note on output file. Tests/running the code should not write to the source tree, except perhaps the current directory.
tests/test_file_processing.py
Outdated
| @@ -16,7 +17,11 @@ def sample_pdf(tmp_path): | |||
|
|
|||
| def test_pdf_converter(caplog, sample_pdf): | |||
| caplog.set_level(logging.INFO) | |||
| file_id = "test_file" | |||
There was a problem hiding this comment.
I have refactored this test. Please use pytest and its fixtures instead of unit-test. And avoid classes for testing unless absolutely necessary. Setup/teardown should ideally be through fixture.
Please don't remove tests. Add new tests freely but removing tests should be done with careful consideration... it generally indicates that API compatibility has been broken. This is early in the codebase development so the tests are toytests for now. Even still, you removed some useful validation rather than just adding additional validation... I combined the asserts made in both test versions that you wrote.
There was a problem hiding this comment.
Got it! We'll start using pytest and its fixtures from now on and avoid using classes for testing unless really needed.
We understand the importance of keeping existing tests for API compatibility. Our goal wasn't to remove valuable tests but to improve validation. We'll bring back the useful validations and merge them with the new assertions to ensure thorough test coverage.
tests/test_file_processing.py
Outdated
| runner = CliRunner() | ||
| result = runner.invoke(pdf_xml, [str(sample_pdf), "test_file"]) | ||
| result = runner.invoke(pdf_xml, [str(sample_pdf), file_id]) |
There was a problem hiding this comment.
I have kept the runner invocation the same.
If you want to test the root command (I named that "osm", then add a separate test for that). As mentioned elsewhere the cli/pdf conversion functionality should ideally be tested independently to ensure separation of concerns.
…pre-commit, and clean up code (docker, server) before XML conversion
supercedes #20.
Rebased version of previous PR
Loom video
https://www.loom.com/share/2b2dfbd8ef334f8280ed676dbde66b4b?sid=f5e951bb-2362-434c-baae-b02fdee8bd17
How to test
pip install -e .docker run --rm -p 8070:8070 elifesciences/sciencebeam-parserand keep it running for the rest of the testing periodosm pdf-xml-json "example_pdf_inputs/test_sample.pdf" {the output file}Loom Video https://www.loom.com/share/d4d3819fce9c439d9799eee4c4a9cc22?sid=f8fc9515-dbd9-4ebb-89d1-b2e0ba1abffd
How to build the Docker image and run the Docker container
Navigate to the project's root directory and run docker-compose up --build
When the image is built and the containers are running, open another terminal and start the osm container in interactive mode using the command docker-compose run osm bash
You can do file conversions in the container using this command osm pdf-xml-json "path_to_file_name.pdf" output_file_path
Or use the command docker-compose run --rm osm osm pdf-xml-json "path_to_file_name.pdf" output_file_path to convert files in non-interactive mode