Skip to content

Issue #13809: Kill mutation in Tree-Walker-2#13976

Merged
rnveach merged 1 commit into
checkstyle:masterfrom
Kevin222004:treeWalker-2
Nov 13, 2023
Merged

Issue #13809: Kill mutation in Tree-Walker-2#13976
rnveach merged 1 commit into
checkstyle:masterfrom
Kevin222004:treeWalker-2

Conversation

@Kevin222004

Copy link
Copy Markdown
Contributor

Issue #13809: Kill mutation in Tree-Walker-2

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java Outdated
Comment thread src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java
@romani romani force-pushed the treeWalker-2 branch 6 times, most recently from 5e0c2d4 to 82b3283 Compare November 11, 2023 00:03
@romani romani marked this pull request as ready for review November 11, 2023 00:04
@romani romani requested a review from rnveach November 11, 2023 00:05

@romani romani 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.

ok to merge

strange, on pushing from local a bit different code but Github is not showing it.
consider to merge #14015 instead.

@romani romani force-pushed the treeWalker-2 branch 2 times, most recently from 82b3283 to 556acfb Compare November 11, 2023 02:08
@rnveach

rnveach commented Nov 11, 2023

Copy link
Copy Markdown
Contributor

#14015
#13976

One of these need to be closed or explained why they look so identical.

@romani

romani commented Nov 11, 2023

Copy link
Copy Markdown
Member

There was some problem with GitHub, it didn't show in PR my latest push, so I created new PR, looks like now all is good.

@rnveach

rnveach commented Nov 12, 2023

Copy link
Copy Markdown
Contributor

* This test is required for pitest coverage.
* Order of Check execution is not visible from outside
* as we order violations by line and column.
* This test is checking that Checks execution ordered by name and then by id.

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.

Order of Check execution is not visible from outside

This is somewhat incorrect. This code came from random ordering of Javadoc checks. Multiple checks printed the same violation, but different executions picked which check was being printed as the source of the violation. This would cause differences in difference reports as we display who is the source of the violation.

The specific code from pitesting shows, we order Checks by class name, id, and then by hashcode. Those are the ordering of the execution of checks. Order of violations is defined elsewhere.

ordered by name and then by id.

Show how this test ensures checks are ordered by id too? I only see somewhat reliance on check name ordering.

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.

The whole javadoc should just be:

This test is checking that Checks execution ordered by name.

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.

applied.

final Checker checker = createChecker(treeWalkerConfig);

try {
checker.process(files);

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.

Why can't this be a BDD test? I am not seeing anything really preventing it.

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.

All tries to make it in bad style did not kill mutation. Because mutation on order of checks, order of checks is not publicly visible.
I tried bunch and gave up.

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.

Have we tried something like the following?

  • config with two checks
  • both checks have bad config (maybe property name)
  • we assert that it is the first check in the config that reported in the exception
    ?

@romani romani Nov 12, 2023

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.

I am not , but exception will happen before Check is added to sorted-set of Checks of Treewker.
Order of check to load from config is defined by xml, predictable order, but mutation is for order of execution in validation runtime (traverse of tree).

I tried to keep test clear that first execution on beginTree will throw exception, so it very clear definition that Checks are ordered.
After bunch of time with this mutation, I am very ok to keep it as non bdd, because final variant of test is still easy to understand, I hope.

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.

Yes, test is easy to understand.

@nrmancuso nrmancuso 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.

A hack for sure, but let's not spend too much time on this.

@rnveach rnveach merged commit 6af8ecf into checkstyle:master Nov 13, 2023
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.

4 participants