[Merged by Bors] - Add ambiguity detection tests#6053
[Merged by Bors] - Add ambiguity detection tests#6053joseph-gio wants to merge 14 commits intobevyengine:mainfrom
Conversation
alice-i-cecile
added
A-ECS
|
Looks good to me so far. Let me know when you're ready for review. |
BorisBoutillier
left a comment
BorisBoutillier
left a comment
There was a problem hiding this comment.
I have mainly reviewed the tests, to see if I could understand them ( and thus the ambiguity cases).
|
|
||
| let mut test_stage = SystemStage::parallel(); | ||
| test_stage | ||
| .add_system(empty_system) |
There was a problem hiding this comment.
At first, while trying to understand each test and its expected output, I wondered why this system (empty_system) was added for this test.
(and I cloned your repository to run the test with report_ambiguities, to be sure I was not misunderstanding something with the expected value of 3).
So, personally, I would not have added empty_system here, or added a comment to say it should not conflict.
But is also fine as is, of course.
There was a problem hiding this comment.
I didn't write these tests, I just made them compile haha. I'm not entirely sure what the motivation was for adding the empty system, but it may have been to make sure it doesn't flag a false positive somehow. This would probably make more sense as its own test, though.
There was a problem hiding this comment.
Yep, that was the intent. It should be split out into it's own test though.
There was a problem hiding this comment.
Actually, I don't think it even needs its own test. This case is already covered in read_only, so I'll just remove the empty system from read_world.
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
bors r+ |
# Objective - Add unit tests for ambiguity detection reporting. - Incremental implementation of #4299. ## Solution - Refactor ambiguity detection internals to make it testable. As a bonus, this should make it easier to extend in the future. ## Notes * This code was copy-pasted from #4299 and modified. Credit goes to @alice-i-cecile and @afonsolage, though I'm not sure who wrote what at this point.
|
Build failed: |
|
blocked on #6063 |
|
bors retry |
# Objective - Add unit tests for ambiguity detection reporting. - Incremental implementation of #4299. ## Solution - Refactor ambiguity detection internals to make it testable. As a bonus, this should make it easier to extend in the future. ## Notes * This code was copy-pasted from #4299 and modified. Credit goes to @alice-i-cecile and @afonsolage, though I'm not sure who wrote what at this point.
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
# Objective - Add unit tests for ambiguity detection reporting. - Incremental implementation of bevyengine#4299. ## Solution - Refactor ambiguity detection internals to make it testable. As a bonus, this should make it easier to extend in the future. ## Notes * This code was copy-pasted from bevyengine#4299 and modified. Credit goes to @alice-i-cecile and @afonsolage, though I'm not sure who wrote what at this point.
# Objective - Add unit tests for ambiguity detection reporting. - Incremental implementation of bevyengine#4299. ## Solution - Refactor ambiguity detection internals to make it testable. As a bonus, this should make it easier to extend in the future. ## Notes * This code was copy-pasted from bevyengine#4299 and modified. Credit goes to @alice-i-cecile and @afonsolage, though I'm not sure who wrote what at this point.
# Objective - Add unit tests for ambiguity detection reporting. - Incremental implementation of bevyengine#4299. ## Solution - Refactor ambiguity detection internals to make it testable. As a bonus, this should make it easier to extend in the future. ## Notes * This code was copy-pasted from bevyengine#4299 and modified. Credit goes to @alice-i-cecile and @afonsolage, though I'm not sure who wrote what at this point.
5 participants
Objective
Solution
Notes