Skip to content

Conversation

@gaborkaszab
Copy link
Collaborator

FileIOTracker related functionality in RESTSessionCatalog code isn't covered with tests. The PR adds test coverage, and also some adjustments to make the FileIOTracker available in tests.

FileIOTracker related functionality in RESTSessionCatalog code
isn't covered with tests. The PR adds test coverage, and also
some adjustments to make the FileIOTracker available in tests.
@github-actions github-actions bot added the core label Oct 20, 2025
@gaborkaszab gaborkaszab requested a review from nastra October 20, 2025 14:38
@nastra
Copy link
Contributor

nastra commented Oct 20, 2025

@gaborkaszab what's the reason we want to test the track with the rest catalog? We do already have TestFileIOTracker where we test the basic functionality of the tracker

@gaborkaszab
Copy link
Collaborator Author

Hey @nastra ,
Thanks for taking a look! In my opinion the unit tests for TestFileIOTracker are required but not entirely enough. There is untested code in RESTSessionCatalog where we have no detection of a regression if someone messes up the usage of the tracker on the catalog side. I was working on this areas of the code for the client-side table cache for the freshness-aware table loading and I had a false impression that everything is fine with my ongoing changes because the tests passed, however, I did it wrong with the tracker and was surprised there are no test coverage that detects this. Also, with my mentioned freshness loading change I'd like to add further tests relevant for IO tracking together with the table cache and this PR is the first step for that.
Hope this makes sense!

@nastra
Copy link
Contributor

nastra commented Oct 21, 2025

Hey @nastra , Thanks for taking a look! In my opinion the unit tests for TestFileIOTracker are required but not entirely enough. There is untested code in RESTSessionCatalog where we have no detection of a regression if someone messes up the usage of the tracker on the catalog side. I was working on this areas of the code for the client-side table cache for the freshness-aware table loading and I had a false impression that everything is fine with my ongoing changes because the tests passed, however, I did it wrong with the tracker and was surprised there are no test coverage that detects this. Also, with my mentioned freshness loading change I'd like to add further tests relevant for IO tracking together with the table cache and this PR is the first step for that. Hope this makes sense!

I'm currently undecided on this, so let's see what others think

@gaborkaszab
Copy link
Collaborator Author

FYI, this change in freshness-aware loading made me think testing this area has value. But in general, the usage of FileIOTracker inRESTSessionCatalog is untested and some test coverage still makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants