[Merged by Bors] - bevy_reflect: Add compile fail tests for bevy_reflect #7041

MrGVSV · 2022-12-27T20:31:32Z

Objective

There isn't really a way to test that code using bevy_reflect compiles or doesn't compile for certain scenarios. This would be especially useful for macro-centric PRs like #6511 and #6042.

Solution

Using bevy_ecs_compile_fail_tests as reference, added the bevy_reflect_compile_fail_tests crate.

Currently, this crate contains a very simple test case. This is so that we can get the basic foundation of this crate agreed upon and merged so that more tests can be added by other PRs.

Open Questions

Should this be added to CI? (Answer: Yes)

Changelog

Added the bevy_reflect_compile_fail_tests crate for testing compilation errors

alice-i-cecile · 2022-12-27T20:58:23Z

This definitely needs to be added to CI. I also don't think this is controversial. @nicopap what was your reasoning? :)

MrGVSV · 2022-12-27T21:00:56Z

Let me know if I made those CI changes incorrectly or not. I basically just copied the ECS compile fail stuff

.gitignore

alice-i-cecile · 2022-12-27T21:04:14Z

CI changes look correct to me, but @mockersf may have complaints.

tools/ci/src/main.rs

mockersf · 2022-12-27T21:38:49Z

I chose to keep it nested like this to keep the crates dir clean

I would prefer to have this one at the same level than the ecs compile fail tests

james7132 · 2022-12-27T21:47:06Z

I would prefer to have this one at the same level than the ecs compile fail tests

I'd advocate for a dedicated directory for these kinds of tests and others, like larger integration and E2E tests, which we may need in the further future as the number of engine features and their interactions start cross-pollinating.

MrGVSV · 2022-12-27T22:10:32Z

I would prefer to have this one at the same level than the ecs compile fail tests

I'd advocate for a dedicated directory for these kinds of tests and others, like larger integration and E2E tests, which we may need in the further future as the number of engine features and their interactions start cross-pollinating.

Should I move both into a single directory in this PR then? And if so, is it one crate or just one directory containing multiple crates?

james7132 · 2022-12-27T22:38:48Z

Should I move both into a single directory in this PR then? And if so, is it one crate or just one directory containing multiple crates?

Up to you. I think it adds a lot of noise and should probably be done in it's own PR. The top level tests directory is basically unused right now. It could go there, but it we could also use another directory if someone else has a stronger opinion about this.

alice-i-cecile · 2022-12-27T23:39:44Z

I'd prefer it live in tests, but moved in a seperate PR.

MrGVSV · 2022-12-28T05:05:53Z

Moved the crate into crates/ from where I originally had it in crates/bevy_reflect.

nicopap · 2022-12-28T07:27:37Z

@alice-i-cecile The change updating top level files, I thought this necessarily fell into "heightened standard of approval" as per the CONTRIBUTING.md my bad if I misinterpreted it.

alice-i-cecile · 2022-12-28T14:12:28Z

Ah understandable. In this case @cart has already endorsed the idea of compile fail tests in general and for bevy_reflect in particular, and this is the consistent way to implement these.

.gitignore

alice-i-cecile · 2022-12-29T02:45:19Z

Ping @MrGVSV on the gitgnore question: it's easy to miss in the resolved comment thread.

cart · 2023-01-02T21:04:31Z

bors r+

# Objective There isn't really a way to test that code using bevy_reflect compiles or doesn't compile for certain scenarios. This would be especially useful for macro-centric PRs like #6511 and #6042. ## Solution Using `bevy_ecs_compile_fail_tests` as reference, added the `bevy_reflect_compile_fail_tests` crate. Currently, this crate contains a very simple test case. This is so that we can get the basic foundation of this crate agreed upon and merged so that more tests can be added by other PRs. ### Open Questions - [x] Should this be added to CI? (Answer: Yes) --- ## Changelog - Added the `bevy_reflect_compile_fail_tests` crate for testing compilation errors

bors · 2023-01-02T21:24:54Z

Pull request successfully merged into main.

Build succeeded!

And happy new year! 🎉

# Objective There isn't really a way to test that code using bevy_reflect compiles or doesn't compile for certain scenarios. This would be especially useful for macro-centric PRs like bevyengine#6511 and bevyengine#6042. ## Solution Using `bevy_ecs_compile_fail_tests` as reference, added the `bevy_reflect_compile_fail_tests` crate. Currently, this crate contains a very simple test case. This is so that we can get the basic foundation of this crate agreed upon and merged so that more tests can be added by other PRs. ### Open Questions - [x] Should this be added to CI? (Answer: Yes) --- ## Changelog - Added the `bevy_reflect_compile_fail_tests` crate for testing compilation errors

MrGVSV added A-Reflection Runtime information about types C-Testing A change that impacts how we test Bevy or how users test their apps labels Dec 27, 2022

nicopap approved these changes Dec 27, 2022

View reviewed changes

nicopap added the X-Controversial There is active debate or serious implications around merging this PR label Dec 27, 2022

MrGVSV mentioned this pull request Dec 27, 2022

bevy_reflect: Ignored field order #6511

Closed

alice-i-cecile approved these changes Dec 27, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 27, 2022

MrGVSV force-pushed the reflect-compile-fail-foundations branch from 87908b2 to 2d8f6f9 Compare December 27, 2022 21:00

alice-i-cecile reviewed Dec 27, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 27, 2022

mockersf reviewed Dec 27, 2022

View reviewed changes

tools/ci/src/main.rs Outdated Show resolved Hide resolved

mockersf reviewed Dec 27, 2022

View reviewed changes

tools/ci/src/main.rs Outdated Show resolved Hide resolved

MrGVSV force-pushed the reflect-compile-fail-foundations branch from 2d8f6f9 to 4404c68 Compare December 28, 2022 05:03

MrGVSV requested review from james7132 and mockersf December 28, 2022 05:04

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed X-Controversial There is active debate or serious implications around merging this PR labels Dec 28, 2022

MrGVSV force-pushed the reflect-compile-fail-foundations branch from 4404c68 to 32cecbd Compare December 28, 2022 17:55

alice-i-cecile approved these changes Dec 28, 2022

View reviewed changes

james7132 approved these changes Dec 28, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

MrGVSV added 3 commits December 28, 2022 20:55

Add compile fail tests for bevy_reflect

385edaf

Add bevy_reflect_compile_fail_tests to CI

84e3442

Add README to bevy_reflect_compile_fail_tests

5c9efbe

MrGVSV force-pushed the reflect-compile-fail-foundations branch from 32cecbd to 5c9efbe Compare December 29, 2022 04:56

james7132 approved these changes Dec 29, 2022

View reviewed changes

cart approved these changes Jan 2, 2023

View reviewed changes

bors bot changed the title ~~bevy_reflect: Add compile fail tests for bevy_reflect~~ [Merged by Bors] - bevy_reflect: Add compile fail tests for bevy_reflect Jan 2, 2023

bors bot closed this Jan 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - bevy_reflect: Add compile fail tests for bevy_reflect #7041

[Merged by Bors] - bevy_reflect: Add compile fail tests for bevy_reflect #7041

MrGVSV commented Dec 27, 2022 •

edited

Loading

alice-i-cecile commented Dec 27, 2022

MrGVSV commented Dec 27, 2022

alice-i-cecile commented Dec 27, 2022

mockersf commented Dec 27, 2022

james7132 commented Dec 27, 2022

MrGVSV commented Dec 27, 2022

james7132 commented Dec 27, 2022 •

edited

Loading

alice-i-cecile commented Dec 27, 2022

MrGVSV commented Dec 28, 2022

nicopap commented Dec 28, 2022

alice-i-cecile commented Dec 28, 2022

alice-i-cecile commented Dec 29, 2022

cart commented Jan 2, 2023

bors bot commented Jan 2, 2023

[Merged by Bors] - bevy_reflect: Add compile fail tests for bevy_reflect #7041

[Merged by Bors] - bevy_reflect: Add compile fail tests for bevy_reflect #7041

Conversation

MrGVSV commented Dec 27, 2022 • edited Loading

Objective

Solution

Open Questions

Changelog

alice-i-cecile commented Dec 27, 2022

MrGVSV commented Dec 27, 2022

alice-i-cecile commented Dec 27, 2022

mockersf commented Dec 27, 2022

james7132 commented Dec 27, 2022

MrGVSV commented Dec 27, 2022

james7132 commented Dec 27, 2022 • edited Loading

alice-i-cecile commented Dec 27, 2022

MrGVSV commented Dec 28, 2022

nicopap commented Dec 28, 2022

alice-i-cecile commented Dec 28, 2022

alice-i-cecile commented Dec 29, 2022

cart commented Jan 2, 2023

bors bot commented Jan 2, 2023

MrGVSV commented Dec 27, 2022 •

edited

Loading

james7132 commented Dec 27, 2022 •

edited

Loading