Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #13809: Kill mutation in PackageObjectFactory #13924

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

Kevin222004
Copy link
Contributor

Issue #13809: Kill mutation in PackageObjectFactory

Explaination

if the string contains packageSeparator then the instance will be null. Probably no test case is possible.

@Kevin222004 Kevin222004 force-pushed the PackageObjectFactory branch 2 times, most recently from 635061a to f8573f7 Compare October 22, 2023 08:08
@romani
Copy link
Member

romani commented Oct 22, 2023

CI is timeouting during test execution, that is weird, please investigate.

@romani
Copy link
Member

romani commented Oct 26, 2023

Rebased by GitHub

@rnveach
Copy link
Member

rnveach commented Oct 26, 2023

I thought we mentioned this area before. This is another time saving area.

If name provided has a period in it, then it is always a full class path. They will never be in createFromStandardCheckSet and createObjectFromClassPath and they will always return null.

I don't agree with the way the code was changed.

@romani
Copy link
Member

romani commented Oct 26, 2023

This update not merge able for sure, CI can not pass.

@rdiachenko
Copy link
Contributor

rdiachenko commented Oct 30, 2023

@Kevin222004
Instead of changing the main logic, could you try to update this test testCreateObjectWithNameContainingPackageSeparator as follows:

    @Test
    public void testCreateObjectWithNameContainingPackageSeparator() throws Exception {
        final ClassLoader classLoader = ClassLoader.getSystemClassLoader();
        final Set<String> packages = Collections.singleton(BASE_PACKAGE);
        final PackageObjectFactory objectFactory =
            new PackageObjectFactory(packages, classLoader, TRY_IN_ALL_REGISTERED_PACKAGES);

        try (MockedStatic<ModuleReflectionUtil> utilities =
                     mockStatic(ModuleReflectionUtil.class)) {
            utilities.when(() -> ModuleReflectionUtil.getCheckstyleModules(packages, classLoader))
                    .thenThrow(new IllegalStateException("mock exception"));

            final Object object = objectFactory.createModule(MockClass.class.getName());
            assertWithMessage("Object should be an instance of MockClass")
                    .that(object)
                    .isInstanceOf(MockClass.class);
        }
    }

When this test is executed, !name.contains(PACKAGE_SEPARATOR) condition is skipped, thus, the code which throws mocked exception is also skipped. On the other hand, when pitest replaces this condition with true, the logic inside is executed and mocked exception is thrown which kills mutation.

@romani
Copy link
Member

romani commented Oct 31, 2023

thenThrow(new IllegalStateException("mock exception"))

thenThrow(new IllegalStateException("creation of objects by fully qualified names Shou not call for search of modules in classpath"))

And we need to add comment above test method to explain why we need this test.

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Items

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

I updated wording a bit.

ok to merge if CI is green.

Copy link
Contributor

@rdiachenko rdiachenko left a comment

Choose a reason for hiding this comment

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

lgtm

@rdiachenko rdiachenko assigned Vyom-Yadav and unassigned rdiachenko Nov 3, 2023
@romani romani merged commit 1282fe6 into checkstyle:master Nov 3, 2023
112 checks passed
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.

None yet

5 participants