Skip to content

[fix][test] Replace usage of org.testcontainers.shaded.* classes in tests#20437

Merged
lhotari merged 2 commits into
apache:masterfrom
lhotari:lh-replace-shaded-awaitility
May 30, 2023
Merged

[fix][test] Replace usage of org.testcontainers.shaded.* classes in tests#20437
lhotari merged 2 commits into
apache:masterfrom
lhotari:lh-replace-shaded-awaitility

Conversation

@lhotari

@lhotari lhotari commented May 30, 2023

Copy link
Copy Markdown
Member

Motivation

org.testcontainers.shaded.* classes shouldn't be used.

Modifications

  • replace usage of org.testcontainers.shaded.* classes in tests with the correct imports.
  • organize imports

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@lhotari lhotari added this to the 3.1.0 milestone May 30, 2023
@lhotari lhotari self-assigned this May 30, 2023
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label May 30, 2023
tisonkun
tisonkun approved these changes May 30, 2023
View reviewed changes

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

nicoloboschi
nicoloboschi reviewed May 30, 2023
View reviewed changes

@nicoloboschi nicoloboschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about adding a new checkstyle rule to avoid the import of

org.testcontainers.shaded ?

@lhotari

lhotari commented May 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

What about adding a new checkstyle rule to avoid the import of

org.testcontainers.shaded ?

@nicoloboschi We have already #18113 and I also tried lhotari@03624d2a since there was a comment somewhere that IllegalImport check doesn't apply to class imports.
It appears that we don't run checkstyle for tests so that's why it doesn't work.

UPDATE: We do have checkstyle for tests enabled:

pulsar/pom.xml

Line 1922 in e380910

<includeTestSourceDirectory>true</includeTestSourceDirectory>

I'll check why my experiment didn't work.

@lhotari

lhotari commented May 30, 2023

Copy link
Copy Markdown
Member Author

Found it. Most checks are disabled for tests:

pulsar/buildtools/src/main/resources/pulsar/suppressions.xml

Line 41 in 82237d3

<suppress checks="^(?!.*UnusedImports).*$" files=".*[\\/]src[\\/]test[\\/].*"/>

@lhotari

lhotari commented May 30, 2023

Copy link
Copy Markdown
Member Author

@nicoloboschi The checkstyle rule is now active and will prevent importing org.testcontainers.shaded.* classes in the future.

@lhotari lhotari requested a review from nicoloboschi May 30, 2023 08:07
@tisonkun

tisonkun commented May 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

thought - I'd actually prefer to use some technologies that can automatically format code like spotless (see FLINK-20651) so we end style bikeshedding :D, but it can introduce a big diff and the maven plugin cannot cover "forbid import pattern" pattern (while the Gralde one can, I suppose it's technically possible upstream to support for maven plugin also).

I know that such issues prevent typical contributors from touching Pulsar's code.

@lhotari lhotari merged commit 5f2c29d into apache:master May 30, 2023
nodece pushed a commit to nodece/pulsar that referenced this pull request Mar 12, 2024
@lhotari @nodece
[fix][test] Replace usage of org.testcontainers.shaded.* classes in t…
bbf814c 
…ests (apache#20437)

(cherry picked from commit 5f2c29d)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tisonkun tisonkun tisonkun approved these changes

@Demogorgon314 Demogorgon314 Demogorgon314 approved these changes

@nicoloboschi nicoloboschi nicoloboschi approved these changes

@poorbarcode poorbarcode poorbarcode approved these changes

@coderzc coderzc Awaiting requested review from coderzc

Assignees

@lhotari lhotari

Labels

area/test doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Milestone

3.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

@lhotari @tisonkun @Demogorgon314 @nicoloboschi @poorbarcode