Remove unproven #131

brainstorm · 2020-12-26T00:58:18Z

Fixes #116

@MarcoIeni @burrbull Let's do what you suggest on #116, shall we? See the integration test mostly borrowed from @MarcoIeni's svd_parser

Getting the following error for now, ~~seems like getting the same capitalization (and perhaps ordering?) might be challenging for integration testing purposes~~... nope, it's just <fields /> being empty, I guess the SVD parser should tolerate that?:

    <peripheral>
           <name>RNG</name>
           <description>RNG register</description>
           <baseAddress>0x3FF20E44</baseAddress>
           <addressBlock>
                       <offset>0</offset>
                       <size>32</size>
                       <usage>RNG register</usage>
           </addressBlock>
           <registers>
                  <register>
                       <name>rng</name>
                       <fields />
                       <description>RNG register</description>
                       <addressOffset>0</addressOffset>
                       <size>32</size>
                       <access>read-only</access>
                      <resetValue>0</resetValue>
                   </register>
        </registers>
     </peripheral>

---- read_and_write stdout ----
thread 'read_and_write' panicked at 'Failed to parse the SVD file into Rust structs: In device `esp8266`

Caused by:
    0: In peripheral `RNG`
    1: In register `rng`
    2: Register have `fields` tag, but it is empty', tests/integration.rs:14:38
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

brainstorm · 2020-12-26T02:39:02Z

A couple of bits I've realized while thinking about this PR:

Ideally this integration testing should be run against https://github.com/posborne/cmsis-svd, to expose this parser to many ICs, even those not even supported by @rust-embedded, since they don't have HALs/PACs (yet)... if so, would it make sense to pull the repo via git submodules or other mechanisms are preferred? Perhaps this is already done somewhere else (in svdtools)? EDIT: CI seems to be thoroughly discussed on Testing svdtools#6... only for STM32 though.
I mentioned the introduction of a lax (or strict) feature flag in e17cbae ... I've seen the pros/cons mentioned in passing somewhere else, but it would be good to reference those discussions here and reach consensus to get it done for good?

brainstorm · 2020-12-26T03:48:42Z

tests/integration.rs

+    assert_eq!(device_svd, device_svd_deserialized)
+}
+
+// fn diff_svds(orig: Path, dest: Path) -> Result<Diff> {


@MarcoIeni, Are you exploring https://github.com/MarcoIeni/delta for SVD diffing currently?

no, I did some work on delta just for fun, not svd related reasons :P

burrbull · 2020-12-26T06:58:03Z

1. Ideally this integration testing should be run against https://github.com/posborne/cmsis-svd, to expose this parser to many ICs, even those not even supported by @rust-embedded, since they don't have HALs/PACs (yet)... if so, would it make sense to pull the repo via git submodules or other mechanisms are preferred? Perhaps this is already done somewhere else (in [svdtools](https://github.com/stm32-rs/svdtools/issues/9))? **EDIT:** CI seems to be thoroughly discussed on [stm32-rs/svdtools#6](https://github.com/stm32-rs/svdtools/issues/6)...  only for STM32 though.

cmsis-svd is already connected via submodules. Or I don't understand question.

2. I mentioned the introduction of a `lax` (or `strict`) feature flag in [e17cbae](https://github.com/rust-embedded/svd/commit/e17cbae99667dcd9d681ecbb80313a64b02f477e) ... I've seen the pros/cons mentioned in passing somewhere else, but it would be good to reference those discussions here and reach consensus to get it done for good?

Yes, we need to add strict feature for several new checks.
CMSIS SVD specification says that some tags can absent, but can't be empty. So we should have such checks, but can hide them under feature.
https://www.keil.com/pack/doc/CMSIS/SVD/html/elem_registers.html#elem_fields

brainstorm · 2020-12-26T08:07:41Z

cmsis-svd is already connected via submodules. Or I don't understand question.

Ouch, my fault for not doing a git clone --recursive, just realised now 🤦🏻 Thanks.

2. I mentioned the introduction of a `lax` (or `strict`) feature flag in [e17cbae](https://github.com/rust-embedded/svd/commit/e17cbae99667dcd9d681ecbb80313a64b02f477e) ... I've seen the pros/cons mentioned in passing somewhere else, but it would be good to reference those discussions here and reach consensus to get it done for good?
Yes, we need to add strict feature for several new checks.
CMSIS SVD specification says that some tags can absent, but can't be empty. So we should have such checks, but can hide them under feature.
https://www.keil.com/pack/doc/CMSIS/SVD/html/elem_registers.html#elem_fields

Good, is that being worked on under some branch?

burrbull · 2020-12-26T08:22:55Z

Good, is that being worked on under some branch?

No, as I know.

You can see this also:
rust-embedded/svd2rust#447

brainstorm · 2020-12-26T09:31:20Z

Ah, good to see the integration tests there, I suspected this might not be the right place to put those, thanks for the pointer!

Shall I remove the integration test from here and you merge this PR with just the unproven flag removed or just close this PR and let you carry on? ;)

burrbull · 2020-12-26T09:53:20Z

Ah, good to see the integration tests there, I suspected this might not be the right place to put those, thanks for the pointer!

Shall I remove the integration test from here and you merge this PR with just the unproven flag removed or just close this PR and let you carry on? ;)

1 think 3 PRs should be here:

remove unproven
hide some checks under strict
add more tests

brainstorm · 2020-12-26T10:22:35Z

Alright, backtracked on this PR so that it just fits point 1: remove unproven.

Cheers!

MarcoIeni · 2020-12-26T10:33:23Z

tests/bad_svd.rs

@@ -4,7 +4,7 @@ use svd_parser as svd;
 fn arm_sample_faulty() {
    let xml = include_str!(concat!(
        env!("CARGO_MANIFEST_DIR"),
-        "/tests/ARM_Sample_faulty.svd"
+        "/tests/data/ARM_Sample_faulty.svd"


you missed this I think

You mean reverting it as it was? I think that this structure is more intuitive (keep tests and data separate)... it's a minimal change and tests keep passing as they were anyway?:

$ cargo test (...) running 1 test test arm_sample_faulty ... ok test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Sorry, I haven't seen you moved the file!

MarcoIeni · 2020-12-26T10:34:36Z

.gitignore

@@ -2,4 +2,3 @@
 [._]*.sw[a-p]
 Cargo.lock
 target
-tests/


is there a reason why tests directory was ignored?

🤷🏻‍♂️ ... I guess that @burrbull was generating some files automatically and did not want to version them in the past? Can't see any reason to keep ignoring them now TBH, that's why I removed the dir from .gitignore.

It is not my repo initially :)

burrbull · 2020-12-26T10:46:50Z

LGTM. Squash commits and modify changelog please.

MarcoIeni · 2020-12-26T10:49:01Z

Squash commits and modify changelog please.

Just out of curiosity. Why should he squash commits? You can select the option "Squash and merge" from github.

burrbull · 2020-12-26T10:50:43Z

We use bors for tests and merging as I remember.

MarcoIeni · 2020-12-26T10:51:26Z

you are right, thanks!

burrbull · 2020-12-26T10:53:59Z

bors r+

@MarcoIeni

131: Remove unproven r=burrbull a=brainstorm Fixes #116 @MarcoIeni @burrbull Let's do what you suggest on #116, shall we? See the integration [test mostly borrowed from @MarcoIeni's `svd_parser`](https://github.com/rust-embedded/svd/pull/131/files#diff-eb771f54a1fdc1c330a82981d7d27d1202a5cbe3a59581ff67af5e3361f5f90e) Getting the following error for now, ~seems like getting the same capitalization (and perhaps ordering?) might be challenging for integration testing purposes~... nope, it's just `<fields />` being empty, I guess the SVD parser should tolerate that?: ```xml <peripheral> <name>RNG</name> <description>RNG register</description> <baseAddress>0x3FF20E44</baseAddress> <addressBlock> <offset>0</offset> <size>32</size> <usage>RNG register</usage> </addressBlock> <registers> <register> <name>rng</name> <fields /> <description>RNG register</description> <addressOffset>0</addressOffset> <size>32</size> <access>read-only</access> <resetValue>0</resetValue> </register> </registers> </peripheral> ``` ```rust ---- read_and_write stdout ---- thread 'read_and_write' panicked at 'Failed to parse the SVD file into Rust structs: In device `esp8266` Caused by: 0: In peripheral `RNG` 1: In register `rng` 2: Register have `fields` tag, but it is empty', tests/integration.rs:14:38 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` Co-authored-by: Roman Valls Guimera <[email protected]>

bors · 2020-12-26T10:58:07Z

Build failed:

continuous-integration/travis-ci/push

burrbull · 2020-12-26T11:00:03Z

Looks like ci is broken. We need to move it on Github Actions and update cross

MarcoIeni · 2020-12-26T11:01:10Z

Looks like ci is broken. We need to move it on Github Actions and update cross

See #132
I can do it!

burrbull · 2020-12-26T20:16:48Z

bors r+

bors · 2020-12-26T20:19:03Z

Build succeeded:

ForsakenHarmony · 2021-02-21T07:16:32Z

We use bors for tests and merging as I remember.

You can configure bors to squash merge

MarcoIeni · 2021-02-21T08:44:52Z

You can configure bors to squash merge

Yes, but I don't remember where, therealprof told me that sometimes they prefer to keep commit history, so they don't want to squash and merge everytime.

brainstorm marked this pull request as ready for review December 26, 2020 02:30

brainstorm requested a review from a team as a code owner December 26, 2020 02:30

brainstorm commented Dec 26, 2020

View reviewed changes

brainstorm changed the title ~~Remove unproven, draft integration tests~~ Remove unproven Dec 26, 2020

MarcoIeni reviewed Dec 26, 2020

View reviewed changes

MarcoIeni mentioned this pull request Dec 26, 2020

Is Continuous Integration working? #132

Closed

Remove unproven flag. Bump up rayon and once_cell.

799fb33

burrbull approved these changes Dec 26, 2020

View reviewed changes

bors bot merged commit 8a8b7a7 into rust-embedded:master Dec 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unproven #131

Remove unproven #131

brainstorm commented Dec 26, 2020 •

edited

Loading

brainstorm commented Dec 26, 2020 •

edited

Loading

brainstorm Dec 26, 2020

MarcoIeni Dec 26, 2020

burrbull commented Dec 26, 2020 •

edited

Loading

brainstorm commented Dec 26, 2020 •

edited

Loading

burrbull commented Dec 26, 2020

brainstorm commented Dec 26, 2020 •

edited

Loading

burrbull commented Dec 26, 2020

brainstorm commented Dec 26, 2020

MarcoIeni Dec 26, 2020

brainstorm Dec 26, 2020

MarcoIeni Dec 26, 2020

MarcoIeni Dec 26, 2020

brainstorm Dec 26, 2020 •

edited

Loading

burrbull Dec 26, 2020

burrbull commented Dec 26, 2020

MarcoIeni commented Dec 26, 2020

burrbull commented Dec 26, 2020

MarcoIeni commented Dec 26, 2020

burrbull commented Dec 26, 2020

bors bot commented Dec 26, 2020

burrbull commented Dec 26, 2020

MarcoIeni commented Dec 26, 2020

burrbull commented Dec 26, 2020

bors bot commented Dec 26, 2020

ForsakenHarmony commented Feb 21, 2021

MarcoIeni commented Feb 21, 2021

Remove unproven #131

Remove unproven #131

Conversation

brainstorm commented Dec 26, 2020 • edited Loading

brainstorm commented Dec 26, 2020 • edited Loading

brainstorm Dec 26, 2020

Choose a reason for hiding this comment

MarcoIeni Dec 26, 2020

Choose a reason for hiding this comment

burrbull commented Dec 26, 2020 • edited Loading

brainstorm commented Dec 26, 2020 • edited Loading

burrbull commented Dec 26, 2020

brainstorm commented Dec 26, 2020 • edited Loading

burrbull commented Dec 26, 2020

brainstorm commented Dec 26, 2020

MarcoIeni Dec 26, 2020

Choose a reason for hiding this comment

brainstorm Dec 26, 2020

Choose a reason for hiding this comment

MarcoIeni Dec 26, 2020

Choose a reason for hiding this comment

MarcoIeni Dec 26, 2020

Choose a reason for hiding this comment

brainstorm Dec 26, 2020 • edited Loading

Choose a reason for hiding this comment

burrbull Dec 26, 2020

Choose a reason for hiding this comment

burrbull commented Dec 26, 2020

MarcoIeni commented Dec 26, 2020

burrbull commented Dec 26, 2020

MarcoIeni commented Dec 26, 2020

burrbull commented Dec 26, 2020

bors bot commented Dec 26, 2020

burrbull commented Dec 26, 2020

MarcoIeni commented Dec 26, 2020

burrbull commented Dec 26, 2020

bors bot commented Dec 26, 2020

ForsakenHarmony commented Feb 21, 2021

MarcoIeni commented Feb 21, 2021

brainstorm commented Dec 26, 2020 •

edited

Loading

brainstorm commented Dec 26, 2020 •

edited

Loading

burrbull commented Dec 26, 2020 •

edited

Loading

brainstorm commented Dec 26, 2020 •

edited

Loading

brainstorm commented Dec 26, 2020 •

edited

Loading

brainstorm Dec 26, 2020 •

edited

Loading