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

Maintenance/update amethyst test #1882

Merged
merged 10 commits into from Aug 16, 2019

Conversation

@azriel91
Copy link
Member

commented Aug 15, 2019

Description

Updated amethyst_test::AmethystApplication to take both System and SystemDesc

(currently sits on top of #1881 so that CI succeeds).

Additions

  • AmethystApplication takes in SystemDescs through with_system_desc.
  • AmethystApplication::with_thread_local_desc takes in RunNowDesc.

Modifications

  • AmethystApplication takes in a System instead of a closure for with_system.
  • AmethystApplication::with_thread_local constraint relaxed to RunNow (previously System).

PR Checklist

By placing an x in the boxes I certify that I have:

  • Updated the content of the book if this PR would make the book outdated.
  • Added a changelog entry if this will impact users, or modified more than 5 lines of Rust that wasn't a doc comment.
  • n/a Added unit tests for new code added in this PR.
  • Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme.

If this modified or created any rs files:

  • Ran cargo +stable fmt --all
  • Ran cargo clippy --all --features "empty"
  • Ran cargo test --all --features "empty"
amethyst_test/src/amethyst_application.rs Outdated Show resolved Hide resolved
@codecov

This comment has been minimized.

Copy link

commented Aug 15, 2019

Codecov Report

Merging #1882 into master will decrease coverage by 1.35%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1882      +/-   ##
==========================================
- Coverage    81.1%   79.75%   -1.36%     
==========================================
  Files          71       70       -1     
  Lines        5240     5205      -35     
==========================================
- Hits         4250     4151      -99     
- Misses        990     1054      +64
Impacted Files Coverage Δ
amethyst_test/src/lib.rs 0% <ø> (ø) ⬆️
amethyst_derive/tests/test.rs 98.64% <ø> (ø) ⬆️
amethyst_test/src/system_injection_bundle.rs 100% <100%> (+16.66%) ⬆️
amethyst_test/src/amethyst_application.rs 97.66% <100%> (+0.79%) ⬆️
amethyst_test/src/state/custom_dispatcher_state.rs 100% <100%> (+1.88%) ⬆️
amethyst_gltf/src/format/mesh.rs 0% <0%> (-94.12%) ⬇️
amethyst_utils/src/ortho_camera.rs 87.5% <0%> (-12.5%) ⬇️
amethyst_error/src/lib.rs 82.94% <0%> (-12.41%) ⬇️
amethyst_audio/src/output.rs 96.87% <0%> (-3.13%) ⬇️
amethyst_locale/src/lib.rs
... and 1 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 c0263f4...15d4b5a. Read the comment docs.

@azriel91 azriel91 referenced this pull request Aug 16, 2019
7 of 7 tasks complete
@jojolepro

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

bors bot added a commit that referenced this pull request Aug 16, 2019
Merge #1882
1882: Maintenance/update amethyst test r=distransient,jojolepro a=azriel91

## Description

Updated `amethyst_test::AmethystApplication` to take both `System` and `SystemDesc`

(currently sits on top of #1881 so that CI succeeds).

## Additions

* `AmethystApplication` takes in `SystemDesc`s through `with_system_desc`.
* `AmethystApplication::with_thread_local_desc` takes in `RunNowDesc`.

## Modifications

* `AmethystApplication` takes in a `System` instead of a closure for `with_system`.
* `AmethystApplication::with_thread_local` constraint relaxed to `RunNow` (previously `System`).

## PR Checklist

By placing an x in the boxes I certify that I have:

- [x] Updated the content of the book if this PR would make the book outdated.
- [x] Added a changelog entry if this will impact users, or modified more than 5 lines of Rust that wasn't a doc comment.
- **n/a** Added unit tests for new code added in this PR.
- [x] Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme.

If this modified or created any rs files:

- [x] Ran `cargo +stable fmt --all`
- [x] Ran `cargo clippy --all --features "empty"`
- [x] Ran `cargo test --all --features "empty"`


Co-authored-by: Azriel Hoh <azriel91@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

@bors bors bot merged commit 15d4b5a into amethyst:master Aug 16, 2019

4 checks passed

bors Build succeeded
Details
codecov/patch 100% of diff hit (target 81.1%)
Details
codecov/project Absolute coverage decreased by -1.35% but relative coverage increased by +18.89% compared to c0263f4
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@azriel91 azriel91 deleted the azriel91:maintenance/update-amethyst-test branch Aug 16, 2019

bors bot added a commit that referenced this pull request Aug 18, 2019
Merge #1883
1883: Improvement/system desc ergonomics r=jojolepro a=azriel91

## Description

Extend `SystemDesc` derive to generate implementation for system with event channel reader fields. Essentially:

```patch
-#[derive(Debug)]
+#[derive(Debug, SystemDesc)]
+#[system_desc(name(SystemWithEventChannelDesc))]
 struct SystemWithEventChannel {
+    #[system_desc(event_channel_reader)]
     u32_reader: ReaderId<u32>,
 }

-use amethyst_core::SystemDesc;
-
-/// Builds a `SystemWithEventChannel`.
-#[derive(Default, Debug)]
-pub struct SystemWithEventChannelDesc;
-
-impl<'a, 'b> SystemDesc<'a, 'b, SystemWithEventChannel> for SystemWithEventChannelDesc {
-    fn build(self, world: &mut World) -> SystemWithEventChannel {
-        <SystemWithEventChannel as System<'_>>::SystemData::setup(world);
-
-        let u32_reader = world.fetch_mut::<EventChannel<u32>>().register_reader();
-
-        SystemWithEventChannel::new()
-    }
-}
```

**Note:** built on top of #1882, so build is green. See commit 932a58d onwards

## Modifications

* `SystemDesc` proc macro supports `#[system_desc(event_reader_id)]` to register event reader.

## PR Checklist

By placing an x in the boxes I certify that I have:

- [x] Updated the content of the book if this PR would make the book outdated.
- [x] Added a changelog entry if this will impact users, or modified more than 5 lines of Rust that wasn't a doc comment.
- [x] Added unit tests for new code added in this PR.
- [x] Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme.

If this modified or created any rs files:

- [x] Ran `cargo +stable fmt --all`
- [x] Ran `cargo clippy --all --features "empty"`
- [x] Ran `cargo test --all --features "empty"`


Co-authored-by: Azriel Hoh <azriel91@gmail.com>
bors bot added a commit that referenced this pull request Aug 18, 2019
Merge #1883
1883: Improvement/system desc ergonomics r=jojolepro a=azriel91

## Description

Extend `SystemDesc` derive to generate implementation for system with event channel reader fields. Essentially:

```patch
-#[derive(Debug)]
+#[derive(Debug, SystemDesc)]
+#[system_desc(name(SystemWithEventChannelDesc))]
 struct SystemWithEventChannel {
+    #[system_desc(event_channel_reader)]
     u32_reader: ReaderId<u32>,
 }

-use amethyst_core::SystemDesc;
-
-/// Builds a `SystemWithEventChannel`.
-#[derive(Default, Debug)]
-pub struct SystemWithEventChannelDesc;
-
-impl<'a, 'b> SystemDesc<'a, 'b, SystemWithEventChannel> for SystemWithEventChannelDesc {
-    fn build(self, world: &mut World) -> SystemWithEventChannel {
-        <SystemWithEventChannel as System<'_>>::SystemData::setup(world);
-
-        let u32_reader = world.fetch_mut::<EventChannel<u32>>().register_reader();
-
-        SystemWithEventChannel::new()
-    }
-}
```

**Note:** built on top of #1882, so build is green. See commit 932a58d onwards

## Modifications

* `SystemDesc` proc macro supports `#[system_desc(event_reader_id)]` to register event reader.

## PR Checklist

By placing an x in the boxes I certify that I have:

- [x] Updated the content of the book if this PR would make the book outdated.
- [x] Added a changelog entry if this will impact users, or modified more than 5 lines of Rust that wasn't a doc comment.
- [x] Added unit tests for new code added in this PR.
- [x] Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme.

If this modified or created any rs files:

- [x] Ran `cargo +stable fmt --all`
- [x] Ran `cargo clippy --all --features "empty"`
- [x] Ran `cargo test --all --features "empty"`


Co-authored-by: Azriel Hoh <azriel91@gmail.com>
@azriel91 azriel91 referenced this pull request Aug 19, 2019
5 of 5 tasks complete
bors bot added a commit that referenced this pull request Aug 20, 2019
Merge #1891
1891: Maintenance/use strings for dispatcher operation r=jojolepro a=azriel91

## Description

Store system name and dependencies as `String`s in `DispatcherOperation`. #1882 broke backward compatibility in `amethyst_test` by requiring the name and dependencies to be `'static`.

cc: @AndreaCatania

## Modifications

* `DispatcherOperation` stores system name and dependencies as `String`s.

## PR Checklist

By placing an x in the boxes I certify that I have:

- **n/a** Updated the content of the book if this PR would make the book outdated.
- [x] Added a changelog entry if this will impact users, or modified more than 5 lines of Rust that wasn't a doc comment.
- **n/a** Added unit tests for new code added in this PR.
- [x] Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme.

If this modified or created any rs files:

- [x] Ran `cargo +stable fmt --all`
- [x] Ran `cargo clippy --all --features "empty"`
- [x] Ran `cargo test --all --features "empty"`


Co-authored-by: Azriel Hoh <azriel91@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.