Skip to content

test: add xls with oom allocations#529

Closed
sftse wants to merge 1 commit intotafia:masterfrom
sftse:oom-alloc
Closed

test: add xls with oom allocations#529
sftse wants to merge 1 commit intotafia:masterfrom
sftse:oom-alloc

Conversation

@sftse
Copy link
Copy Markdown
Contributor

@sftse sftse commented Jul 14, 2025

This is the last part of #463 not yet folded into other PRs.

This adds a file that OOM allocates. If the test is run this will result in an abort.
There doesn't seem to be a sensible fix that would make the file readable, as the file seems corrupted when opening with Libreoffice and when looking at the CFB structures using the cfb crate.

Because allocator failures are still being treated as aborts by Rust (which is likely to change in the future), there's no way to catch these calamine failures as a user of the library without resorting to nightly.

@jmcnamara
Copy link
Copy Markdown
Collaborator

@sftse This isn't really, or currently, a Pull Request. Could you convert it to a bug report instead.

@sftse
Copy link
Copy Markdown
Contributor Author

sftse commented Sep 4, 2025

This was originally thought of as a PR, would you need a fix to consider it complete?

@jmcnamara
Copy link
Copy Markdown
Collaborator

This was originally thought of as a PR, would you need a fix to consider it complete?

Maybe I'm missing something, but is this a failing test case? I can't tell because the CI failed for other reasons. If it is a failing test then it shouldn't be merged without a fix.

@sftse
Copy link
Copy Markdown
Contributor Author

sftse commented Sep 4, 2025

We cannot run the test in CI because it aborts and kills the test harness. This is inconvenient because one cannot see exactly what test failed and what would have passed, unlike panics in tests where you still get whether the test case was good or failed.

Since I cannot fix this at the time, I thought there may be some value in merging this anyway, then it becomes code owned by the repo and somebody may fix it. At least for me, I hesitate to go in and fix things related to a PR that is open, so an open PR that is not quite there can be more of a hindrance.

The point of this merge would be to track the file, but not execute it, CI is then fine. But I see how this could be a bit strange.

@jmcnamara
Copy link
Copy Markdown
Collaborator

The point of this merge would be to track the file, but not execute it, CI is then fine. But I see how this could be a bit strange.

Ok. I get it now.

@jmcnamara
Copy link
Copy Markdown
Collaborator

Closing in favour of the bug report at #550

@jmcnamara jmcnamara closed this Sep 4, 2025
@sftse sftse deleted the oom-alloc branch September 10, 2025 10:32
@sftse sftse mentioned this pull request Oct 29, 2025
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