tmt icon indicating copy to clipboard operation
tmt copied to clipboard

Fix cryptic unit parsing error in Data Size Fields

Open skycastlelily opened this issue 3 months ago • 7 comments

This mr partially fixes #4273, the tmt run part, but not the tmt lint part, cause when users run "tmt lint", normalize_* are not called, despite repeated confirmation from Claude and TMT Code Buddy that they were being called.Thank Milos for quickly identifying this issue after several minutes' investigation, more mr(s) will be sent.

Pull Request Checklist

  • [x] implement the feature
  • [ ] write the documentation
  • [ ] extend the test coverage
  • [ ] update the specification
  • [ ] adjust plugin docstring
  • [ ] modify the json schema
  • [ ] mention the version
  • [ ] include a release note

skycastlelily avatar Nov 05 '25 14:11 skycastlelily

LGTM. My only comment is that the normalize_* should be generalized to reduce duplication, but that's for a different PR.

haha, Milos has the same concern, gonna to send a mr to address that.

skycastlelily avatar Nov 06 '25 14:11 skycastlelily

LGTM. My only comment is that the normalize_* should be generalized to reduce duplication, but that's for a different PR.

haha, Milos has the same concern, gonna to send a mr to address that.

I'd maybe switch the order of these PRs, because here you are adding code the next PR will remove. Change the order, and will not need to remove code that has not been added because those two functions no longer contain their copy of code, but use a shared helper instead.

happz avatar Nov 06 '25 15:11 happz

LGTM. My only comment is that the normalize_* should be generalized to reduce duplication, but that's for a different PR.

haha, Milos has the same concern, gonna to send a mr to address that.

I'd maybe switch the order of these PRs, because here you are adding code the next PR will remove. Change the order, and will not need to remove code that has not been added because those two functions no longer contain their copy of code, but use a shared helper instead.

addressed :sweat_drops:

skycastlelily avatar Nov 07 '25 07:11 skycastlelily

@skycastlelily a test, even a small one, and a release note would be nice.

happz avatar Nov 10 '25 07:11 happz

Please make a comment or reset the reviews and link the relevant dependent PR when you update the branch since Github doesn't seem to notify about that change (I think it does if it is automatically moved due to a PR being merged).

Sure :)

LGTM, about the testing, it would be really good to add it, especially since here we care about the order of when this validation fails as per the related issue.

addressed: https://github.com/teemtee/tmt/pull/4285/commits/6dbc4ede0093c71be59e11f7967383b74c5ba353

skycastlelily avatar Nov 10 '25 14:11 skycastlelily

@skycastlelily a test, even a small one, and a release note would be nice.

Sure, https://github.com/teemtee/tmt/pull/4285/commits/6dbc4ede0093c71be59e11f7967383b74c5ba353 ^^

skycastlelily avatar Nov 10 '25 15:11 skycastlelily

Regarding the tests, good to have unit test coverage, but it doesn't really cover the breakage of #4273. For that one we need an explicit tmt test somewhere in /tests/provision/hardware that checks that we exit as early as possible and we do not enter even the discover phase.

Addressed https://github.com/teemtee/tmt/commit/c90dcf8ae8ebc5a18d5f7dc6cf1a6f15204d3f35 :)

skycastlelily avatar Nov 11 '25 07:11 skycastlelily

@skycastlelily, please update the pull request checklist and move the release note to the docs/releases/pending/ folder. Thanks.

psss avatar Nov 24 '25 15:11 psss

@skycastlelily, please update the pull request checklist and move the release note to the docs/releases/pending/ folder. Thanks.

sure,and done.

skycastlelily avatar Nov 25 '25 03:11 skycastlelily

@skycastlelily please, inspect the tests, there seems to be a problem with PintError.

happz avatar Nov 26 '25 20:11 happz

@skycastlelily please, inspect the tests, there seems to be a problem with PintError.

hmmm, cannot import name 'PintError' from 'pint.errors' (/usr/lib/python3.9/site-packages/pint/errors.py)" error, because PintError does not exit in python3.9, should we replace PintError with Union[UndefinedUnitError, DimensionalityError]?:)

skycastlelily avatar Nov 28 '25 03:11 skycastlelily

@skycastlelily please, inspect the tests, there seems to be a problem with PintError.

hmmm, cannot import name 'PintError' from 'pint.errors' (/usr/lib/python3.9/site-packages/pint/errors.py)" error, because PintError does not exit in python3.9, should we replace PintError with Union[UndefinedUnitError, DimensionalityError]?:)

That is quite unfortunate, especially since it seems we are the only consumers of python-pint and there is no reason to not update it

$ fedrq wrsrc python-pint -b epel9 -F source
tmt

In the meantime, I think it's fine to just put it as except Exception in those places instead

LecrisUT avatar Nov 28 '25 08:11 LecrisUT

hmmm, cannot import name 'PintError' from 'pint.errors' (/usr/lib/python3.9/site-packages/pint/errors.py)" error, because PintError does not exit in python3.9, should we replace PintError with Union[UndefinedUnitError, DimensionalityError]?:)

That is quite unfortunate, especially since it seems we are the only consumers of python-pint and there is no reason to not update it

$ fedrq wrsrc python-pint -b epel9 -F source
tmt

In the meantime, I think it's fine to just put it as except Exception in those places instead

Thanks ,updated :)

skycastlelily avatar Nov 28 '25 14:11 skycastlelily

/packit test --id provision:virtual

happz avatar Dec 02 '25 12:12 happz

/packit build

psss avatar Dec 03 '25 11:12 psss