Skip to content

Issue #13809: Kill mutation ConfigurationLoader2#13900

Merged
rnveach merged 1 commit intocheckstyle:masterfrom
Kevin222004:ConfigurationLoader2
Nov 7, 2023
Merged

Issue #13809: Kill mutation ConfigurationLoader2#13900
rnveach merged 1 commit intocheckstyle:masterfrom
Kevin222004:ConfigurationLoader2

Conversation

@Kevin222004
Copy link
Copy Markdown
Contributor

@Kevin222004 Kevin222004 commented Oct 18, 2023

Issue #13809: Kill mutation ConfigurationLoader2

Test

@Kevin222004 Kevin222004 mentioned this pull request Oct 18, 2023
Closed
33 tasks
romani
romani requested changes Oct 19, 2023
View reviewed changes
Copy link
Copy Markdown
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

@Kevin222004 Kevin222004 force-pushed the ConfigurationLoader2 branch 2 times, most recently from 0c056e1 to 8e5d371 Compare October 25, 2023 09:41
@romani
Copy link
Copy Markdown
Member

romani commented Oct 26, 2023

Kevin requested help on my comments , we need to help him.

@romani romani force-pushed the ConfigurationLoader2 branch from 1e73851 to 8e5d371 Compare October 29, 2023 14:23
@romani
Copy link
Copy Markdown
Member

romani commented Oct 29, 2023 •
edited
Loading

your code is correct by it is really hard to understand why this works like this.

lets make test full functional.

<?xml version="1.0" encoding="UTF-8"?>

<!DOCTYPE module PUBLIC
    "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
    "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <module name="TreeWalker">
        <module name="MemberName">
            <property name="severity" value="x${severity}"  default="ignore"/>
        </module>
    </module>
</module>

load this config as it is done at

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/ConfigurationLoaderTest.java

Line 81 in bd3a9bb

public void testResourceLoadConfiguration() throws Exception {

and confirm that property is set to default value.

@romani romani force-pushed the ConfigurationLoader2 branch from 8e5d371 to 62ca8b0 Compare October 29, 2023 14:40
@romani romani force-pushed the ConfigurationLoader2 branch 2 times, most recently from 12e2e6d to 877fb72 Compare November 5, 2023 14:48
romani
romani approved these changes Nov 5, 2023
View reviewed changes
Copy link
Copy Markdown
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 code to required state

@romani romani force-pushed the ConfigurationLoader2 branch from 877fb72 to ceefdd6 Compare November 5, 2023 14:52
@romani
Copy link
Copy Markdown
Member

romani commented Nov 5, 2023

Github, generate website

@romani romani assigned rnveach and unassigned romani Nov 5, 2023
@romani romani requested a review from rnveach November 5, 2023 14:53
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 5, 2023

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/ceefdd6_2023152903/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/ceefdd6_2023152903/config.html#

rnveach
rnveach reviewed Nov 5, 2023
View reviewed changes
src/test/java/com/puppycrawl/tools/checkstyle/ConfigurationLoaderTest.java
}

@Test
public void testDefaultValuesForNonDefinedProperties() throws Exception {
Copy link
Copy Markdown
Contributor

@rnveach rnveach Nov 5, 2023

Choose a reason for hiding this comment

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

Why can't this test be written in our normal format?

@romani Don't we require javadoc for custom tests?

Copy link
Copy Markdown
Member

@romani romani Nov 5, 2023

Choose a reason for hiding this comment

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

Whole test class is not using our general format. I still do not feel good to migrate this also to our general for modules format.

Tests are very similar to each other.

src/xdocs/config.xml

<p>
ATTENTION: default value is applied to module property value if at least one expansion
property is not defined.
Copy link
Copy Markdown
Contributor

@rnveach rnveach Nov 5, 2023

Choose a reason for hiding this comment

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

I assume this is referring to property values with multiple expansions in a single property?

This needs to be expanded. In addition why don't we add an example?

Copy link
Copy Markdown
Member

@romani romani Nov 5, 2023

Choose a reason for hiding this comment

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

Not only multiple expansion, any .

Copy link
Copy Markdown
Member

@romani romani Nov 5, 2023

Choose a reason for hiding this comment

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

I thought about example, but it is not easy to explain and might be too technical, and looks like nobody dealing with this, so we found this nuance only during pitest.

I think we don't need to make our doc to be too much extended and hard to read.

@rnveach rnveach merged commit ab7ac57 into checkstyle:master Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@romani romani romani approved these changes

+1 more reviewer

@rnveach rnveach rnveach left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@rnveach rnveach

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Kevin222004 @romani @rnveach