search

patternfly / pf-codemods

Codemods for upgrading from react-core@4.x.x to react-core@5.x.x. Uses eslint.
11 stars 18 forks source link

Investigate disparate behavior in unit tests and real world use #704

Open wise-king-sullyman opened 3 months ago

wise-king-sullyman commented 3 months ago

Follow up to #675

In order to land #675 I commented out some tests that were failing because of an issue with our test setup.

Nested test components are not updated in the unit tests, but are properly handled by the codemod in the real world.

Ideally we should dig into why we run into these false negative tests, and resolve the issue.

Additionally we should expand the text-replace-with-content test suite with coverage for alternative imports, aliasing, etc.

thatblindgeye commented 3 months ago

We should also update the codemod to check for TextProps and TextVariants imports and update them accordingly to ContentProps and ContentVariants, per updates made in https://github.com/patternfly/patternfly-react/pull/10643/files#diff-d30d90a7adc9371640f6e7ec412615f846adc6dd3bc39243b48d55f3506c2fd1

wise-king-sullyman commented 3 months ago

Made #706 for the previous comment

adamviktora commented 3 months ago

I believe the tests are setup correctly. The issue lies in how the ESLint rules are executed. According to the documentation, after the fix is applied, it will run the rule (apply the fix) on the fixed code again and again until there is no diff between the files (no potential changes to fix) or until the rule runs 10 times https://eslint.org/docs/latest/extend/custom-rules#applying-fixes.

The ESLint tests apply the fix only once. In case of nested components, I believe it is treated as a range conflict, so only one component can be renamed while applying the fix.

This means that https://github.com/patternfly/pf-codemods/pull/675 (text to content codemod) will not work for case where some element is nested more than 10 times, e.g. some deeply nested list items.