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.

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

[Merged by Bors] - Fix compile_fail tests #2984

Closed
wants to merge 2 commits into from

Conversation

MinerSebas
Copy link
Contributor

Objective

Solution

  • Add #[derive(Component)] to the doctest
  • Also changed their cfg attribute from doc to doctest, so that these tests don't appear when running cargo doc with --document-private-items

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Oct 17, 2021
@alice-i-cecile alice-i-cecile added A-Build-System Related to build systems or continuous integration A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change S-Needs-Review and removed S-Needs-Triage This issue needs to be labelled labels Oct 17, 2021
@bjorn3
Copy link
Contributor

bjorn3 commented Oct 17, 2021

Also changed their cfg attribute from doc to doctest, so that these tests don't appear when running cargo doc with --document-private-items

You could use #[doc(hidden)]. That will cause it to be hidden even with --document-private-items.

@MinerSebas
Copy link
Contributor Author

MinerSebas commented Oct 17, 2021

Also changed their cfg attribute from doc to doctest, so that these tests don't appear when running cargo doc with --document-private-items

You could use #[doc(hidden)]. That will cause it to be hidden even with --document-private-items.

I chose against this as #[cfg(doctest)] better represents the intention of the code, compared with #[cfg(doc)] + `#[doc(hidden)]

@mockersf mockersf 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 S-Needs-Review labels Oct 17, 2021
@mockersf
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 18, 2021
# Objective

- Bevy has several `compile_fail` test
- #2254 added `#[derive(Component)]`
- Those tests now fail for a different reason.
- This was not caught as these test still "successfully" failed to compile.

## Solution

- Add `#[derive(Component)]` to the doctest
- Also changed their cfg attribute from `doc` to `doctest`, so that these tests don't appear when running `cargo doc` with `--document-private-items`
@bors bors bot changed the title Fix compile_fail tests [Merged by Bors] - Fix compile_fail tests Oct 18, 2021
@bors bors bot closed this Oct 18, 2021
@MinerSebas MinerSebas deleted the compile-fail branch October 25, 2021 17:28
bors bot pushed a commit that referenced this pull request Nov 13, 2021
# Objective

bevy_ecs has several compile_fail tests that assert lifetime safety. In the past, these tests have been green for the wrong reasons (see e.g. #2984). This PR makes sure, that they will fail if the compiler error changes.

## Solution

Use [trybuild](https://crates.io/crates/trybuild) to assert the compiler errors.

The UI tests are in a separate crate that is not part of the Bevy workspace. This is to ensure that they do not break Bevy's crater builds. The tests get executed by the CI workflow on the stable toolchain.
bors bot pushed a commit that referenced this pull request Nov 13, 2021
# Objective

bevy_ecs has several compile_fail tests that assert lifetime safety. In the past, these tests have been green for the wrong reasons (see e.g. #2984). This PR makes sure, that they will fail if the compiler error changes.

## Solution

Use [trybuild](https://crates.io/crates/trybuild) to assert the compiler errors.

The UI tests are in a separate crate that is not part of the Bevy workspace. This is to ensure that they do not break Bevy's crater builds. The tests get executed by the CI workflow on the stable toolchain.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants