Issue #12099: Add ArchUnit test to detect cycles in packages #12190

Vyom-Yadav · 2022-09-10T17:05:57Z

#12099

Link to documentation about freezing arch rules: https://www.archunit.org/userguide/html/000_Index.html#_configuration

ViolationStore was created using property freeze.store.default.allowStoreCreation=true, default is false as we do not want it to be created in CI pipeline.

example of failure in test from diff: 4b61c26

romani · 2022-09-10T19:07:40Z

How it looks like if we add new cycle ? Or be able to resolve cycle?

You can create more commits in this PR to add remove/add some code to pass compilation and let arch unit to action

romani · 2022-09-11T21:38:36Z

@Vyom-Yadav , please resove conflict.

please create extra test commits to show how it will looks like to deal with it, we can remove such extra commits ones we confirm that all works.

nrmancuso · 2022-09-14T03:07:55Z

please resolve conflict

done

please create extra test commits to show how it will looks like to deal with it, we can remove such extra commits ones we confirm that all works.

From https://dev.azure.com/romanivanovjr/a35b6275-caac-4633-b371-79a5ab0f70c1/_apis/build/builds/11277/logs/60:

FAILURE! - in com.puppycrawl.tools.checkstyle.internal.ArchUnitCyclesCheckTest
2022-09-14T02:57:45.5419884Z [ERROR] testSlicesShouldBeFreeOfCycle  Time elapsed: 2.432 s  <<< FAILURE!
2022-09-14T02:57:45.5420634Z java.lang.AssertionError: 
2022-09-14T02:57:45.5421949Z Architecture Violation [Priority: MEDIUM] - Rule 'slices matching 'com.puppycrawl.tools.checkstyle.(**)' should be free of cycles' was violated (5 times):
2022-09-14T02:57:45.5422807Z Cycle detected: Slice api -> 
2022-09-14T02:57:45.5423465Z                 Slice checks.javadoc -> 
2022-09-14T02:57:45.5423923Z                 Slice api
2022-09-14T02:57:45.5424382Z   1. Dependencies of Slice api
2022-09-14T02:57:45.5425822Z     - Method <com.puppycrawl.tools.checkstyle.api.JavadocUtil.getJavadocTags(com.puppycrawl.tools.checkstyle.api.TextBlock, com.puppycrawl.tools.checkstyle.api.JavadocUtil$JavadocTagType)> has return type <com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocTags> in (JavadocUtil.java:0)
2022-09-14T02:57:45.5429240Z     - Method <com.puppycrawl.tools.checkstyle.api.JavadocUtil.getJavadocTags(com.puppycrawl.tools.checkstyle.api.TextBlock, com.puppycrawl.tools.checkstyle.api.JavadocUtil$JavadocTagType)> calls method <com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocTagInfo.isValidName(java.lang.String)> in (JavadocUtil.java:116)
2022-09-14T02:57:45.5552438Z     - Method <com.puppycrawl.tools.checkstyle.api.JavadocUtil.getJavadocTags(com.puppycrawl.tools.checkstyle.api.TextBlock, com.puppycrawl.tools.checkstyle.api.JavadocUtil$JavadocTagType)> calls constructor <com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocTag.<init>(int, int, java.lang.String, java.lang.String)> in (JavadocUtil.java:118)
2022-09-14T02:57:45.5555091Z     - Method <com.puppycrawl.tools.checkstyle.api.JavadocUtil.getJavadocTags(com.puppycrawl.tools.checkstyle.api.TextBlock, com.puppycrawl.tools.checkstyle.api.JavadocUtil$JavadocTagType)> calls constructor <com.puppycrawl.tools.checkstyle.checks.javadoc.InvalidJavadocTag.<init>(int, int, java.lang.String)> in (JavadocUtil.java:121)
2022-09-14T02:57:45.5557470Z     - Method <com.puppycrawl.tools.checkstyle.api.JavadocUtil.getJavadocTags(com.puppycrawl.tools.checkstyle.api.TextBlock, com.puppycrawl.tools.checkstyle.api.JavadocUtil$JavadocTagType)> calls constructor <com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocTags.<init>(java.util.Collection, java.util.Collection)> in (JavadocUtil.java:125)
...

This is what it looks like when I moved all utils to API in 4b61c26, I think we might not be able to report all violations with the current limits set. I wonder if this affects our ability to detect new violations.

romani · 2022-09-14T12:23:53Z

what was code change ? that caused this error
what is required is we need to reduce or extend known store of suppressions ?

nrmancuso · 2022-09-14T12:33:06Z

what was code change ? that caused this error what is required is we need to reduce or extend known store of suppressions ?

See above comment, I just moved utils to api package to resolve a bunch of violations at one time. The behavior of this freezing rule is strange, see 02e822e where I simply added one violation and have a negative diff of over 1,000 lines in freeze file. I would expect the addition of a few lines at most, not removal of over 1,000.

romani · 2022-09-14T12:44:16Z

last removal in your diff

Method <com.puppycrawl.tools.checkstyle.xpath.XpathQueryGenerator.isMatchingByLineAndColumnAndTokenType(com.puppycrawl.tools.checkstyle.api.DetailAST)> calls method <com.puppycrawl.tools.checkstyle.api.DetailAST.getType()> in (XpathQueryGenerator.java:333)

It is not a cycle, so it might be problem with original generation of file.

nrmancuso · 2022-09-14T12:46:28Z

last removal in your diff

Method <com.puppycrawl.tools.checkstyle.xpath.XpathQueryGenerator.isMatchingByLineAndColumnAndTokenType(com.puppycrawl.tools.checkstyle.api.DetailAST)> calls method <com.puppycrawl.tools.checkstyle.api.DetailAST.getType()> in (XpathQueryGenerator.java:333)

It is not a cycle, so it might be problem with original generation of file.

This looks like all the tool did was remove the newline at the end of the file, unless I am missing something.

Edit: wrong line, I see now

nrmancuso · 2022-09-14T12:55:47Z

Example of removing one violation: ab8dfb8

Diff is removal of over 1,000 lines; also test is still failing after freeze store update, see https://dev.azure.com/romanivanovjr/romanivanovjr/_build/results?buildId=11284&view=logs&j=eb841e9f-6e1e-5c29-3ad5-74ec2c658069

Vyom-Yadav · 2022-09-16T08:53:50Z

We are enabling re-freeze to minimize diff in the store in case a violation is added or removed. This would be more convenient for the developer.

Violation is removed: minor: Remove an ArchUnit Frozen Violation #12205
Violation is added: minor: Add an ArchUnit Violation to the violation store #12206

nrmancuso

Item:

nrmancuso · 2022-09-16T11:20:17Z

src/test/java/com/puppycrawl/tools/checkstyle/internal/ArchUnitCyclesCheckTest.java

+public class ArchUnitCyclesCheckTest {
+
+    @Test
+    public void testSlicesShouldBeFreeOfCycle() {


Please do this: imagine you are a new contributor, and execution of this test has generated a new suppression.

Then, in the issue, place links and information about the concept of this test and links to freeze documentation that would help you to understand why suppression was generated. After that, place a Javadoc here that explains how this test works, and a link to your comment in the issue.

Yes, it good idea to read details in actual test class. Hints on how to interpretate is also good.

What is the process if we ok to allow extra cycle?

@romani @nrmancuso Please take a look at the javadoc.

does it mean that test will always pass and just update freeze file, so will enforce no cycles by simple PR review and noticing a diff - I am ok.

If user forget to place freeze file to commit, CI will fail as it will detect a git-diff.

Yes, for any new violation, the store will be automatically updated, if the user commits the changes in the store, they will be seen and reviewed in the PR.

If user forget to place freeze file to commit, CI will fail as it will detect a git-diff.

Yes

please mention in javadoc that change in freeze file need to be placed in commit.
Any change in freeze file should be committed and shared in PR to let mentors approve new cycle and guide contributor on how to avoid it.

Whenever a new violation is introduced, the test will pass successfully but the frozen violations will be updated. In that case, developers should check if the new violation is expected or not. In both cases create a separate commit in the PR for discussing changes in the violation store.

Added this para to the javadoc.

romani

items

src/test/java/com/puppycrawl/tools/checkstyle/internal/ArchUnitCyclesCheckTest.java

romani

Ok to merge

romani · 2022-09-17T18:08:50Z

@Vyom-Yadav , please resolve conflict

romani

Items

romani · 2022-09-17T18:10:06Z

src/test/java/com/puppycrawl/tools/checkstyle/internal/ArchUnitCyclesCheckTest.java

+     *
+     * <p>Whenever a new cycle is introduced or removed, the test will pass successfully but the
+     * frozen violations will be updated. It is highly recommended to avoid new cycles and, it
+     * is preferred to remove them. In both cases create a separate commit in the PR for discussing


In both cases put update in commit for discussion

What about:
In both cases update the commit for discussing changes in the violation store.

'Update commit' for sounds strange .
We need some sort on "include in commit"

The suppressions generated by this tool are like the metadata generator; most contributors will never know/care about such changes and should commit all changes in one commit, and it is doubtful they will even see this comment until a reviewer points it out. It is responsibility of reviewer to catch extension of freeze file and question why we extended cycles.

Yes, lets agree on some reasonable wording, and let's merge this PR

In both cases commit the changes for further discussion.

romani

Ok to merge

nrmancuso

I am good to merge once CI passes

Vyom-Yadav · 2022-09-17T19:30:53Z

I am good to merge once CI passes

checker-framework will fail due to #12210

romani · 2022-09-17T19:40:36Z

We can not merge .. red CI will affect all

config/archunit-store/stored.rules

src/test/java/com/puppycrawl/tools/checkstyle/internal/ArchUnitCyclesCheckTest.java

config/archunit-store/stored.rules

Vyom-Yadav mentioned this pull request Sep 10, 2022

Expand ArchUnit Testing #12099

Closed

Vyom-Yadav mentioned this pull request Sep 11, 2022

Resolve ArchUnit suppressions #12191

Open

Vyom-Yadav force-pushed the ArchUnit-CycleDetection branch from 743ee96 to c7ee98b Compare September 11, 2022 11:10

nrmancuso force-pushed the ArchUnit-CycleDetection branch from c7ee98b to 4b61c26 Compare September 14, 2022 02:56

nrmancuso force-pushed the ArchUnit-CycleDetection branch from 98c19a9 to 099c889 Compare September 14, 2022 04:55

nrmancuso force-pushed the ArchUnit-CycleDetection branch from 02e822e to ab8dfb8 Compare September 14, 2022 12:53

Vyom-Yadav force-pushed the ArchUnit-CycleDetection branch 4 times, most recently from 3da9267 to 5d49608 Compare September 16, 2022 06:07

nrmancuso requested changes Sep 16, 2022

View reviewed changes

Vyom-Yadav force-pushed the ArchUnit-CycleDetection branch 4 times, most recently from 8939115 to c54f913 Compare September 17, 2022 13:54

romani requested changes Sep 17, 2022

View reviewed changes

src/test/java/com/puppycrawl/tools/checkstyle/internal/ArchUnitCyclesCheckTest.java Outdated Show resolved Hide resolved

src/test/java/com/puppycrawl/tools/checkstyle/internal/ArchUnitCyclesCheckTest.java Outdated Show resolved Hide resolved

Vyom-Yadav force-pushed the ArchUnit-CycleDetection branch from c54f913 to 0c7e410 Compare September 17, 2022 14:18

romani approved these changes Sep 17, 2022

View reviewed changes

romani assigned nrmancuso Sep 17, 2022

romani requested changes Sep 17, 2022

View reviewed changes

Vyom-Yadav force-pushed the ArchUnit-CycleDetection branch 2 times, most recently from 0103dd9 to ec16877 Compare September 17, 2022 19:17

romani approved these changes Sep 17, 2022

View reviewed changes

nrmancuso approved these changes Sep 17, 2022

View reviewed changes

rnveach reviewed Sep 17, 2022

View reviewed changes

Vyom-Yadav force-pushed the ArchUnit-CycleDetection branch from ec16877 to 8da749a Compare September 18, 2022 04:28

rnveach reviewed Sep 18, 2022

View reviewed changes

config/archunit-store/stored.rules Outdated Show resolved Hide resolved

Vyom-Yadav force-pushed the ArchUnit-CycleDetection branch from 8da749a to 7f8e539 Compare September 18, 2022 17:35

Issue checkstyle#12099: Add ArchUnit test to detect cycles in packages

5075bd1

Vyom-Yadav force-pushed the ArchUnit-CycleDetection branch from 7f8e539 to 5075bd1 Compare September 18, 2022 18:16

romani assigned rnveach and unassigned nrmancuso Sep 18, 2022

rnveach approved these changes Sep 18, 2022

View reviewed changes

rnveach merged commit 944f09b into checkstyle:master Sep 18, 2022

This was referenced Sep 19, 2022

minor: Remove an ArchUnit Frozen Violation #12205

Closed

minor: Add an ArchUnit Violation to the violation store #12206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #12099: Add ArchUnit test to detect cycles in packages #12190

Issue #12099: Add ArchUnit test to detect cycles in packages #12190

Vyom-Yadav commented Sep 10, 2022 •

edited by nrmancuso

romani commented Sep 10, 2022

romani commented Sep 11, 2022

nrmancuso commented Sep 14, 2022 •

edited

romani commented Sep 14, 2022 •

edited

nrmancuso commented Sep 14, 2022 •

edited

romani commented Sep 14, 2022

nrmancuso commented Sep 14, 2022 •

edited

nrmancuso commented Sep 14, 2022 •

edited

Vyom-Yadav commented Sep 16, 2022

nrmancuso left a comment

nrmancuso Sep 16, 2022 •

edited

romani Sep 16, 2022

Vyom-Yadav Sep 17, 2022 •

edited

romani Sep 17, 2022

Vyom-Yadav Sep 17, 2022

romani Sep 17, 2022 •

edited

Vyom-Yadav Sep 17, 2022

romani left a comment

romani left a comment

romani commented Sep 17, 2022

romani left a comment

romani Sep 17, 2022

Vyom-Yadav Sep 17, 2022

romani Sep 17, 2022

nrmancuso Sep 17, 2022 •

edited

romani Sep 17, 2022

Vyom-Yadav Sep 17, 2022

romani Sep 17, 2022

Vyom-Yadav Sep 17, 2022

romani left a comment

nrmancuso left a comment

Vyom-Yadav commented Sep 17, 2022

romani commented Sep 17, 2022

Issue #12099: Add ArchUnit test to detect cycles in packages #12190

Issue #12099: Add ArchUnit test to detect cycles in packages #12190

Conversation

Vyom-Yadav commented Sep 10, 2022 • edited by nrmancuso

romani commented Sep 10, 2022

romani commented Sep 11, 2022

nrmancuso commented Sep 14, 2022 • edited

romani commented Sep 14, 2022 • edited

nrmancuso commented Sep 14, 2022 • edited

romani commented Sep 14, 2022

nrmancuso commented Sep 14, 2022 • edited

nrmancuso commented Sep 14, 2022 • edited

Vyom-Yadav commented Sep 16, 2022

nrmancuso left a comment

Choose a reason for hiding this comment

nrmancuso Sep 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vyom-Yadav Sep 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani Sep 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

romani commented Sep 17, 2022

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrmancuso Sep 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

nrmancuso left a comment

Choose a reason for hiding this comment

Vyom-Yadav commented Sep 17, 2022

romani commented Sep 17, 2022

Vyom-Yadav commented Sep 10, 2022 •

edited by nrmancuso

nrmancuso commented Sep 14, 2022 •

edited

romani commented Sep 14, 2022 •

edited

nrmancuso commented Sep 14, 2022 •

edited

nrmancuso commented Sep 14, 2022 •

edited

nrmancuso commented Sep 14, 2022 •

edited

nrmancuso Sep 16, 2022 •

edited

Vyom-Yadav Sep 17, 2022 •

edited

romani Sep 17, 2022 •

edited

nrmancuso Sep 17, 2022 •

edited