Skip to content

Better error messages reading PIDX and PDSC files#721

Merged
bgn42 merged 6 commits intomainfrom
betterErrorMessages
Mar 13, 2026
Merged

Better error messages reading PIDX and PDSC files#721
bgn42 merged 6 commits intomainfrom
betterErrorMessages

Conversation

@bgn42
Copy link
Collaborator

@bgn42 bgn42 commented Mar 13, 2026

Fixes

  • Now the file name is shown if malformed or otherwise bad PIDX or PDSC files are read.

Changes

  • context is added if not yet included in error message

Checklist

  • 🤖 This change is covered by unit tests (if applicable).
  • 🤹 Manual testing has been performed (if necessary).
  • 🛡️ Security impacts have been considered (if relevant).
  • 📖 Documentation updates are complete (if required).
  • 🧠 Third-party dependencies and TPIP updated (if required).

@qltysh
Copy link

qltysh bot commented Mar 13, 2026

All good ✅

@qltysh
Copy link

qltysh bot commented Mar 13, 2026

Qlty

Coverage Impact

⬆️ Merging this pull request will increase total coverage on main by 0.05%.

Modified Files with Diff Coverage (2)

RatingFile% DiffUncovered Line #s
Coverage rating: A Coverage rating: A
cmd/xml/pdsc.go100.0%
Coverage rating: C Coverage rating: C
cmd/utils/utils.go71.4%429-430
Total75.0%
🤖 Increase coverage with AI coding...

In the `betterErrorMessages` branch, add test coverage for this new code:

- `cmd/utils/utils.go` -- Line 429-430

🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 13, 2026

Test Results

    4 files  ± 0     24 suites  ±0   1m 5s ⏱️ +4s
  790 tests + 7    790 ✅ + 7  0 💤 ±0  0 ❌ ±0 
3 143 runs  +28  3 143 ✅ +28  0 💤 ±0  0 ❌ ±0 

Results for commit 0fe82ae. ± Comparison against base commit 2103f46.

♻️ This comment has been updated with latest results.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves error messages when reading PIDX and PDSC XML files by wrapping errors with the relevant filename, so users can identify which file caused the problem. It also provides a clearer message for empty files (EOF "too early").

Changes:

  • Error wrapping logic added to PdscXML.Read(), PidxXML.Read(), and PidxXML.CheckTime() to include filenames in error messages, with guards against double-wrapping.
  • Existing tests updated from assert.Equal to assert.Contains to accommodate the now-enriched error messages, plus new tests covering the wrapping behavior.
  • New MalformedPack.pdsc test fixture added.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
cmd/xml/pdsc.go Wraps ReadXML errors with filename context in PdscXML.Read()
cmd/xml/pidx.go Wraps ReadXML errors with filename context in PidxXML.Read() and CheckTime()
cmd/xml/pdsc_test.go New tests for PDSC error wrapping (XML errors, EOF, nonexistent files)
cmd/xml/pidx_test.go New tests for PIDX error wrapping (XML errors, EOF, double-wrapping, CheckTime)
cmd/installer/root_test.go Loosened assertion from Equal to Contains for enriched error messages
cmd/installer/root_pdsc_add_test.go Loosened assertion from Equal to Contains for enriched error messages
testdata/MalformedPack.pdsc New malformed PDSC test fixture

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves error messages when reading malformed PIDX and PDSC XML files by wrapping XML parsing errors with the filename in ReadXML. It also adds special handling for EOF errors (empty files) with a "too early" annotation.

Changes:

  • ReadXML in cmd/utils/utils.go now wraps decoder.Decode errors with the file path, and annotates EOF errors with "too early"
  • Existing tests are updated from assert.Equal to assert.Contains to accommodate the new error prefix
  • New tests verify filename presence in errors, no double-wrapping, and EOF handling for both PIDX and PDSC paths

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
cmd/utils/utils.go Core change: wraps XML decode errors with the file path
cmd/xml/pdsc.go Refactored Read() to explicit if/err pattern (functionally identical)
cmd/xml/pidx_test.go New tests for error wrapping in PIDX Read and CheckTime
cmd/xml/pdsc_test.go New tests for error wrapping in PDSC Read
cmd/utils/utils_test.go Updated assertion from Equal to Contains
cmd/installer/root_test.go Updated assertion from Equal to Contains
cmd/installer/root_pdsc_add_test.go Updated assertion from Equal to Contains
testdata/MalformedPack.pdsc New malformed PDSC test fixture

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

jkrech
jkrech previously approved these changes Mar 13, 2026
Copy link
Member

@jkrech jkrech left a comment

Choose a reason for hiding this comment

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

I assume that an empty file would report the same error.

Note: If the file is empty, this is not even an xml file and we should not report an XML syntax error, right?

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@bgn42
Copy link
Collaborator Author

bgn42 commented Mar 13, 2026

In case of an empty file the error message would be E: filename: EOF too early

@jkrech
Copy link
Member

jkrech commented Mar 13, 2026

Why not being precise and writing that the file is empty?

@bgn42
Copy link
Collaborator Author

bgn42 commented Mar 13, 2026

For that we would need an extra check before calling the XML decoder. The file is opened and then the XML decoding is called which returns EOF in case where it appears before the syntactically end of that file.

@bgn42
Copy link
Collaborator Author

bgn42 commented Mar 13, 2026

I don't find it particularly helpful when the three of us make changes to the same source code simultaneously.

@brondani
Copy link
Collaborator

I don't find it particularly helpful when the three of us make changes to the same source code simultaneously.

Fully agree, please note I didn't touch the code, I've just answered the request for review.

@brondani brondani dismissed their stale review March 13, 2026 16:13

Regardless of the request, it seems the PR is not yet ready for review.

@bgn42 bgn42 requested a review from jkrech March 13, 2026 16:28
Copy link
Member

@jkrech jkrech left a comment

Choose a reason for hiding this comment

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

LGTM

@bgn42 bgn42 merged commit e4a213c into main Mar 13, 2026
24 checks passed
@bgn42 bgn42 deleted the betterErrorMessages branch March 13, 2026 17:00
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