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.

Sign up for GitHub

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

Jump to bottom

Backwards-compatability tests #506

Closed
Urhengulas opened this issue Jun 11, 2021 · 6 comments · Fixed by #592
Closed

Backwards-compatability tests #506

Urhengulas opened this issue Jun 11, 2021 · 6 comments · Fixed by #592
Assignees
@japaric
Labels
difficulty: easy Pretty easy to solve priority: medium Medium priority for the Knurling team status: needs PR Issue just needs a Pull Request implementing the changes type: question A question that isn't answered by the docs
Milestone
v0.3.0

Comments

@Urhengulas
Copy link
Member

Urhengulas commented Jun 11, 2021 •
edited by japaric
Loading

Status update (2021-09-10) needs implementation. see notes in #506 (comment)

#503 broke the backwards-compatability tests and since we don't guarantee backwards-compatability we kinda disabled them by just running with the current decoder.

My question is now if we should just remove these tests all together since we don't guarantee it anyways, or if we should pin it to the current version of the decoder?
I am fine with both with a slight preference for removing.
@Urhengulas Urhengulas added type: question A question that isn't answered by the docs priority: medium Medium priority for the Knurling team difficulty: easy Pretty easy to solve labels Jun 11, 2021
@Urhengulas
Copy link
Member Author

While thinking about it again I thought that it is already valuable to know how backwards-comaptible our output is. So keeping the tests around gives us this certainty if someone ever comes around and wants to know this.

@jonas-schievink what are your thoughts?

@jonas-schievink
Copy link
Contributor

The purpose of those tests was to make sure that the earliest semver-compatible defmt release can still decode what the current git version produces. The result here does not necessarily have to match 1-to-1 (so the changes to the output were fine), but I can see that that does fail the tests at the moment. Not totally sure how to do this better, maybe we just have to check that the decoder didn't fail?

@Urhengulas
Copy link
Member Author

So I think it would work if we just always install probe-run from main and check that the decoding doesn't fail? Thereby we make sure we don't loose decoding functionality, but enable PR authors to alter the output.

@Urhengulas Urhengulas added the status: needs PR Issue just needs a Pull Request implementing the changes label Jun 28, 2021
@japaric
Copy link
Member

japaric commented Jul 16, 2021

I think we'll need to revise those backcompat tests after #287 is implemented because after that the behavior is going to be:

breaking changes in the encoding / decoding are allowed in patch versions (but breaking API changes are not); using defmt 0.3.x anywhere in the dep graph = you use the latest patch version of defmt = no breakage in code because of the 'no breaking API changes'; probe-run will error (or maybe it will work but not print any logs?) and tell you to upgrade it if it sees a defmt patch version newer than what it supports.

so using the v0.3.x encoder with the 0.3.0 decoder (which the backcompat tests tested) is not something that's going to be part of the workflow anymore

@Urhengulas Urhengulas added this to the v0.3.0 milestone Jul 19, 2021
@Urhengulas Urhengulas mentioned this issue Jul 19, 2021
Merged
bors bot added a commit that referenced this issue Jul 19, 2021
@bors @Urhengulas
Merge #543
9a23d2d 
543: `CI`: Temporarily drop backward-compatibility check r=jonas-schievink a=Urhengulas

Temporarily drop backward-compatibility check (they aren't currently fulfilling their job anyways).

They will get added back in a different form. See #506. 

Co-authored-by: Johann Hemmann <[email protected]>
@Urhengulas
Copy link
Member Author

After #287 we should additionally to the output-format also check backwards-incompatible changes to (quoted from the PR): "the symbol naming scheme, the symbol and section layout, the data encoding / wire format".

@japaric
Copy link
Member

japaric commented Sep 10, 2021

I think what we need to do here to test for changes that break backwards compatibility (after #540) is, at a high level:

  • add a new xtask, say test-backcompat
  • this xtask will run the same set of firmware examples as test-snapshot BUT
    • it will use an "old version" of qemu-run
    • it will not check the output (stdout/stderr) of qemu-run
    • it will check the exit code of qemu-run; it should be 0 (success) in all cases
    • if qemu-run fails, the xtask must indicate which test failed and print whatever qemu-run printed -- so we can figure out where decoding failed

By "old version" of qemu-run I mean a version that uses the oldest git commit of defmt that last bumped this version number.

how to install an "old version" of qemu-run? The easiest way is to run cargo install --git https:/knurling-rs/defmt --rev $git_hash but this is not great for running CI locally because it will override any local qemu-run (though it's unlikely people have that tool installed). An alternative that does not do that would be nice but not a deal breaker.

how to use an "old version" of qemu-run? in the firmware/qemu directory, cargo run is configured (see .cargo/config.toml) to use the local, i.e. latest version, of qemu-run. To override this one can set the CARGO_TARGET_${target_triple}_RUNNER env var. Like this:

$ CARGO_TARGET_THUMBV7M_NONE_EABIHF_RUNNER=path/to/binary cargo run --bin hello

Note that you need to specify the target triple in UPPERCASE letters.

What do we when this test-backcompat test breaks in a pull request?

  • temporarily disable the test-backcompat test to land the PR
  • in a follow-up defmt PR
    • bump the defmt version number in src/lib.rs
    • update xtask to use the commit hash of the pull request merge that broke backcompat

@japaric japaric self-assigned this Sep 27, 2021
@japaric japaric mentioned this issue Sep 29, 2021
Merged
bors bot added a commit that referenced this issue Oct 1, 2021
@bors @japaric
Merge #592
3746241 
592: xtask: add backward compability test r=Urhengulas a=japaric

**UPDATE**: I have left the defmt-version as it is since we have not made a defmt 0.3.0 release. I have also tweaked the defmt-test to not exit with non-zero exit code and remove some test-specific logic from xtask. See individual commits messages for details.

---

implements #506 (comment)
closes #506

I tested this against c7b20d5 (I called that `defmt-version-3` locally) and found that a bitflags change broke the wire format. Not a big deal but I guess we should bump the version to 4? cc `@jonas-schievink` 

``` console
bitflags (dev)
Error: malformed bitflags value string 'Flags::0::FLAG_0'
```

the other thing that I found out is that some of the snapshot test exit with non-zero code:

``` console
defmt-test (dev)
INFO (1/7) running `change_init_struct`...
INFO (2/7) running `test_for_changed_init_struct`...
INFO (3/7) running `assert_true`...
INFO (4/7) running `assert_imported_max`...
INFO (5/7) running `result`...
INFO (6/7) running `should_error`...
INFO (7/7) running `fail`...
ERROR panicked at '`#[should_error]` test failed with outcome: Ok(this should have returned `Err`)'

Timer with period zero, disabling
```

`cargo xtask test-snapshot` is not checking the exit code; it only looks at stdout so that non-zero exit code is not a problem for those tests. in the backcompat test we want to look for decoding errors, not at stdout, so I was using the exit code of `cargo run` but this `defmt-test` is problematic with that check.

Co-authored-by: Jorge Aparicio <[email protected]>
@bors bors bot closed this as completed in 3e2b601 Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@japaric japaric

Labels
difficulty: easy Pretty easy to solve priority: medium Medium priority for the Knurling team status: needs PR Issue just needs a Pull Request implementing the changes type: question A question that isn't answered by the docs
Projects
None yet
Milestone
v0.3.0
Development

Successfully merging a pull request may close this issue.

xtask: add backward compability test knurling-rs/defmt
3 participants
@jonas-schievink @japaric @Urhengulas