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.

Sign up for GitHub

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

Jump to bottom

[java] Make LawOfDemeter results deterministic #4549

Merged
merged 4 commits into from
May 26, 2023
Merged

[java] Make LawOfDemeter results deterministic #4549

merged 4 commits into from
May 26, 2023

Conversation

adangel
Copy link
Member

@adangel adangel commented May 5, 2023

Describe the PR

I believe, this should fix the spurious differing results in the regression tester. At least, I could reproduce on first try the problem when reversing the order of the reaching assignments.

That should then make the test-case-3 of pmd/pmd-regression-tester#119 happy.

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by github actions)
  • Added (in-code) documentation (if needed)

@adangel adangel added the a:bug PMD crashes or fails to analyse a file. label May 5, 2023
@adangel adangel added this to the 7.0.0 milestone May 5, 2023
@ghost
Copy link

ghost commented May 5, 2023 •
edited by ghost
Loading

1 Message
📖 Compared to master:
This changeset changes 11 violations,
introduces 2 new violations, 0 new errors and 0 new configuration errors,
removes 2 violations, 0 errors and 0 configuration errors.
Full report
✅ Compared to master:
This changeset changes 11 violations,
introduces 2 new violations, 0 new errors and 0 new configuration errors,
removes 2 violations, 0 errors and 0 configuration errors.
Full report
✅ Compared to master:
This changeset changes 4 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 12 errors and 0 configuration errors.
Full report
✅ Compared to master:
This changeset changes 4 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 12 errors and 0 configuration errors.
Full report
✅ Compared to master:
This changeset changes 13 violations,
introduces 2 new violations, 0 new errors and 0 new configuration errors,
removes 2 violations, 12 errors and 0 configuration errors.
Full report

Generated by 🚫 Danger

@adangel
Copy link
Member Author

adangel commented May 5, 2023

Ok, the baseline seems to have been created with a different order of the reaching assignments. I'll give it a try and remove the reversed() to see, whether we match the baseline then. Ideally we wouldn't see any changed violations.

@oowekyala
Copy link
Member

oowekyala commented May 5, 2023 •
edited
Loading

Maybe a better fix would be to replace all new HashSet and new HashMap in DataflowPass.java with their LinkedHashX version... Then we wouldn't need the sorting at all, and we ensure other rules won't have this problem.

@adangel @oowekyala
Update pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/desi…
ef3a781 
…gn/LawOfDemeterRule.java

Co-authored-by: Clément Fournier <[email protected]>
@adangel
Copy link
Member Author

adangel commented May 5, 2023

Maybe a better fix would be to replace all new HashSet and new HashMap in DataflowPass.java with their LinkedHashX version... Then we wouldn't need the sorting at all, and we ensure other rules won't have this problem.

I wanted to make it minimally invasive. I can give it a shot, though...

@adangel
Copy link
Member Author

adangel commented May 5, 2023

Maybe a better fix would be to replace all new HashSet and new HashMap in DataflowPass.java with their LinkedHashX version... Then we wouldn't need the sorting at all, and we ensure other rules won't have this problem.

I wanted to make it minimally invasive. I can give it a shot, though...

I've updated the Dataflow code now. I needed to swap the test case again - that means, if this was really the reason for the random results, then we should get the numbers from the first run again:

This changeset changes 13 violations,
introduces 2 new violations, 0 new errors and 0 new configuration errors,
removes 2 violations, 12 errors and 0 configuration errors.

But it should be stable now...

@oowekyala oowekyala merged commit 81233fa into pmd:master May 26, 2023
@adangel adangel deleted the law-of-demeter-deterministic branch May 26, 2023 10:43
@adangel adangel mentioned this pull request May 27, 2023
Merged
adangel added a commit to adangel/pmd that referenced this pull request May 28, 2023
@adangel
[doc] Update release notes for rc3 (pmd#4434, pmd#4549) [skip ci]
8b952c2
@adangel adangel mentioned this pull request Nov 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oowekyala oowekyala oowekyala approved these changes

Assignees
No one assigned
Labels
a:bug PMD crashes or fails to analyse a file.
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@adangel @oowekyala