Issue 524 remove redundant dag cycle check

[ggalmazor]

Closes #524

This PR is built on top of #528 and starts in commit 49bb628

What has been done to verify that this works as intended?

Run the automated tests

Why is this the best possible solution? Were any other approaches considered?

This is a straightforward change:

• First, remove the redundant check. The tests fail due to different exception class and message
• Fix the tests
• Fix the cycle check that's left to have the same behavior thanks to the test harness

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

There shouldn't be any behavior change.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

[codecov-io]

Codecov Report

Merging #526 into master will decrease coverage by 0.04%.
The diff coverage is 76.36%.

@@             Coverage Diff              @@
##             master     #526      +/-   ##
============================================
- Coverage     53.17%   53.13%   -0.05%     
- Complexity     3126     3128       +2     
============================================
  Files           245      245              
  Lines         13370    13374       +4     
  Branches       2572     2573       +1     
============================================
- Hits           7110     7106       -4     
- Misses         5429     5441      +12     
+ Partials        831      827       -4

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/org/javarosa/xform/parse/XFormParser.java	65.17% <ø> (-0.12%)	246 <0> (-1)
...org/javarosa/core/model/condition/Recalculate.java	80% <0%> (-4.22%)	11 <0> (ø)
...a/org/javarosa/core/model/condition/Condition.java	37.83% <0%> (-1.06%)	11 <0> (ø)
...org/javarosa/core/model/condition/Triggerable.java	82.71% <0%> (+1%)	24 <0> (ø)	⬇️
...a/xform/parse/StandardBindAttributesProcessor.java	81.81% <100%> (ø)	25 <0> (ø)	⬇️
...n/java/org/javarosa/core/model/TriggerableDag.java	71.24% <87.5%> (-8.7%)	80 <10> (-6)
...javarosa/core/model/condition/ConditionAction.java	72.72% <91.66%> (-2.28%)	3 <1> (ø)
src/main/java/org/javarosa/core/model/FormDef.java	69.65% <0%> (-0.29%)	155% <0%> (-1%)
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a00caf1...e7abf78. Read the comment docs.

[dcbriccetti]

Hi @lognaturel and @ggalmazor. I’ll be happy to review this, but given this is unfamiliar code I wonder how thorough I should be.

[lognaturel]

I think that after all keeping #528 here makes the most sense and will be easier to deal with. I've moved comments from there to here.

Couple of small things. The most important is to make sure #526 (comment) is right. If I didn't get that right I should spend more time with it.

[lognaturel]

Thanks for looking at this, @dcbriccetti! I think this one @ggalmazor and I can finish up. It's relatively low risk. What would be really fantastic to get your sanity check on is performance analysis of #523. I talked to @ggalmazor today and we now agree that my concerns about memory usage were probably overblown. He's going to write up some analysis and that would be great for you to review.

[ggalmazor]

What would be really fantastic to get your sanity check on is performance analysis of #523. I talked to @ggalmazor today and we now agree that my concerns about memory usage were probably overblown. He's going to write up some analysis and that would be great for you to review.

Yes! The current understanding is that collections reuse object references, which should let us have many more collections (instead of just a single reused collection that gets passed through all the layers) without a noticeable impact in memory usage.

[ggalmazor]

OK, I think the PR is ready for you, @lognaturel :)