-
Notifications
You must be signed in to change notification settings - Fork 23
Gitc 6903 Adding Unit Test Suite #83
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend not putting any files at the repository top level if they are only used for the test suite. I would move them into a test/ directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have more reviewers examine the test cases.
# GDAL_ROOT=$(PREFIX)/src/gdal/gdal | ||
# | ||
include Makefile.lcl | ||
include ../Makefile.lcl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this referencing the makefile for the MRF test suite build? That doesn't make sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, Makefile.lcl does not have any test suite build dependencies. It's a better place for it to be at project root location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this into the test directory rather than doc/Test Suite and not name with all caps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to test directory. Thanks!
|
||
def test_vrt_default_pagesize(self): | ||
"""Test that a default PageSize of 512 is used when the tag is absent.""" | ||
# ARRANGE: Create a 1024x1024 MRF without a <PageSize> tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why 1024 is used here when we're testing the default of 512.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is confirming that missing tag, results in mrf_size.py assuming default of 512x512 for all calculations. To validate, test creates 1024x1024 raster. VRT file generated with dimensions based on number of tiles fit into raster. The script calculates widthxheight of 2x2 tiles, and asserts that output VRT has dimensions of 2x2. The script would fail if default is not 512.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that really how it works? My understanding is that rasterXSize and rasterYSize is the output size of the virtual raster in pixels, not the tile dimensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what mrf_size does, it generates a raster where each pixel corresponds to a input tile, with the pixel value being the size of the matching tile. A 1024x1024 MRF with a tile of 512x512 will generate a 2x2 pixel raster output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't used this in a long time, it's a neat tool to visually identify issues in a very large MRF. The output is graphic, georeferenced and can be vied in any GIS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, interesting. That makes more sense now. I haven't tried the tool before but do see where it can be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I attempted to build the test image with docker build -t mrf-test-suite .
and docker build --platform linux/amd64 -t mrf-test-suite .
and both fail. For the first command, I get the following error:
ERROR: failed to build: failed to solve: process "/bin/sh -c wget -P /tmp/ https://github.com/nasa-gibs/gibs-gdal/releases/download/v${GDAL_VERSION}-${GIBS_GDAL_RELEASE}/gibs-gdal-${GDAL_VERSION}-${GIBS_GDAL_RELEASE}.el${ALMALINUX_VERSION}.x86_64.rpm && dnf install -y /tmp/gibs-gdal-${GDAL_VERSION}-${GIBS_GDAL_RELEASE}.el${ALMALINUX_VERSION}.x86_64.rpm && rm -rf /tmp/*" did not complete successfully: exit code: 1
Investigating the log, I see the following:
Problem: conflicting requests
- package gdal-3.6.4-3.el10.x86_64 from @commandline does not have a compatible architecture
- nothing provides libbrunslidec-c.so()(64bit) needed by gdal-3.6.4-3.el10.x86_64 from @commandline
- nothing provides libbrunslienc-c.so()(64bit) needed by gdal-3.6.4-3.el10.x86_64 from @commandline
- nothing provides libgdal.so.32()(64bit) needed by gdal-3.6.4-3.el10.x86_64 from @commandline
- nothing provides libc.so.6(GLIBC_2.11)(64bit) needed by gdal-3.6.4-3.el10.x86_64 from @commandline
- nothing provides libc.so.6(GLIBC_2.14)(64bit) needed by gdal-3.6.4-3.el10.x86_64 from @commandline
- nothing provides libc.so.6(GLIBC_2.15)(64bit) needed by gdal-3.6.4-3.el10.x86_64 from @commandline
- nothing provides libc.so.6(GLIBC_2.2.5)(64bit) needed by gdal-3.6.4-3.el10.x86_64 from @commandline
- nothing provides libc.so.6(GLIBC_2.3)(64bit) needed by gdal-3.6.4-3.el10.x86_64 from @commandline
- nothing provides libc.so.6(GLIBC_2.3.2)(64bit) needed by gdal-3.6.4-3.el10.x86_64 from @commandline
- nothing provides libc.so.6(GLIBC_2.3.4)(64bit) needed by gdal-3.6.4-3.el10.x86_64 from @commandline
- nothing provides libc.so.6(GLIBC_2.4)(64bit) needed by gdal-3.6.4-3.el10.x86_64 from @commandline
- nothing provides libc.so.6(GLIBC_2.6)(64bit) needed by gdal-3.6.4-3.el10.x86_64 from @commandline
- nothing provides libc.so.6(GLIBC_2.7)(64bit) needed by gdal-3.6.4-3.el10.x86_64 from @commandline
- nothing provides libm.so.6(GLIBC_2.2.5)(64bit) needed by gdal-3.6.4-3.el10.x86_64 from @commandline
(try to add '--skip-broken' to skip uninstallable packages or '--nobest' to use not only best candidate packages)
I am on an M4 mac system, please investigate issues with the dockerfile before merging.
…ilities in mrf_apps
gt[1] *= mrf.pagesize.x | ||
gt[5] *= mrf.pagesize.y | ||
XML.SubElement(root,'GeoTransform').text = ",".join((str(x) for x in gt)) | ||
bands = int(mrf.size.c / mrf.pagesize.c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this should be modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it was correct before. The output is a "tile size" image. When the input is pixel interleaved, there is only one tile per location, so the output has only one band. It's not the same thing as the number of input bands.
# Stage 1: Install all development tools, compile the C++ utilities, | ||
# and create the Python virtual environment. | ||
# ===================================================================== | ||
FROM almalinux:9 AS builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not almalinux:10 ?
Finally, run the tests using the `mrf-test-suite` image. This command starts a container, executes `pytest`, and automatically removes the container (`--rm`) when finished. | ||
|
||
```bash | ||
docker run --rm mrf-test-suite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have tests that might be skipped (and test_jxl_bundle_mode
that's currently guaranteed to be skipped), it might be useful to list the run command here as
docker run --rm mrf-test-suite pytest -rs
so that the user is given reasons if the tests are skipped. Without the pytest -rs
, only the number of skipped tests will be reported, with no clear indication of which tests were skipped or why.
I know you told me to wait until later in the week, but I tried anyways and I was able to successfully build the test image now. When I run the
|
Jackie, to confirm: you build both mrf-app and mrf-test-suite? |
Thanks for the clarification, I built the mrf-app image then the test image and successfully ran the tests. I just failed to read the documentation MRF_Utilities_Test_Suite.md carefully |
Great, thanks for testing Jackie! I'll proceed with upgrading images to AlmaLinux 10. |
Summary
This pull request consists of unit test suite to validate MRF utilities functionality and prevent future regressions. The tests are designed to be run in a self-contained Docker environment.
Tests identified potential bugs that need to be reviewed:
unittest
framework for all C++ and Python utilities.How to Test
The new test suite can be verified by following the instructions in the updated "MRF Utilities Test Suite" documentation.
docker build -t mrf-test-suite .
docker run --rm mrf-test-suite