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

implement CI tests as xtask #393

Merged
merged 70 commits into from
Apr 8, 2021
Merged

implement CI tests as xtask #393

merged 70 commits into from
Apr 8, 2021

Conversation

spookyvision
Copy link
Contributor

@spookyvision spookyvision commented Feb 17, 2021

this xtask implementation fixes #366 and

  • collects all test errors in one grand result vec and displays them at the end
  • has commands for test-all, test-host, test-cross etc.
  • has -k switch to keep targets that were installed as required dependency
  • displays colored diff for failed snapshot tests
  • puts "macos book build fixes" in separate commit ( c847772 ) - please review
  • updates the ci yaml to use this xtask

not done:

  • install os specific packages like libusb (out of scope, IMO)

xtask/src/main.rs Outdated Show resolved Hide resolved
xtask/src/main.rs Outdated Show resolved Hide resolved
xtask/src/main.rs Outdated Show resolved Hide resolved
xtask/src/main.rs Show resolved Hide resolved
@spookyvision spookyvision marked this pull request as draft February 17, 2021 17:40
spookyvision and others added 3 commits March 19, 2021 15:17
Co-authored-by: Johann Hemmann <johann.hemmann@code.berlin>
Co-authored-by: Johann Hemmann <johann.hemmann@code.berlin>
Co-authored-by: Johann Hemmann <johann.hemmann@code.berlin>
Copy link
Member

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked if you missed to transfer any tests, but seems good to me.

fn test_single_snapshot looks a bit messy to me, but from what I understand it does the same as firmware/qemu/test.sh.

One question I am unsure about: Why is the --deny-warnings only looked at in test_host and not the other tests as well?

.github/workflows/ci.yml Outdated Show resolved Hide resolved

let mut features_map = HashMap::new();
features_map.insert("alloc", "alloc");
let no_features = "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const 🎉

xtask/src/main.rs Outdated Show resolved Hide resolved
spookyvision and others added 2 commits March 24, 2021 00:41
Co-authored-by: Johann Hemmann <johann.hemmann@code.berlin>
Co-authored-by: Johann Hemmann <johann.hemmann@code.berlin>
@spookyvision
Copy link
Contributor Author

I checked if you missed to transfer any tests, but seems good to me.

fn test_single_snapshot looks a bit messy to me, but from what I understand it does the same as firmware/qemu/test.sh.

One question I am unsure about: Why is the --deny-warnings only looked at in test_host and not the other tests as well?

@jonas-schievink what do you think?

xtask/src/main.rs Outdated Show resolved Hide resolved
@jonas-schievink
Copy link
Contributor

test_single_snapshot seems okay for now, we can clean it up later though.

One question I am unsure about: Why is the --deny-warnings only looked at in test_host and not the other tests as well?

I don't think there was a particular reason we did this, but if there's a .cargo/config.toml then RUSTFLAGS overrides that, which would break the build, so we can't always set it.

@Urhengulas
Copy link
Member

test_single_snapshot seems okay for now, we can clean it up later though.

One question I am unsure about: Why is the --deny-warnings only looked at in test_host and not the other tests as well?

I don't think there was a particular reason we did this, but if there's a .cargo/config.toml then RUSTFLAGS overrides that, which would break the build, so we can't always set it.

I don't really question the existence of the cmdline-arg, but rather why it is only taken into account for some tests and not also e.g. the test_cross.

Co-authored-by: Jonas Schievink <jonas.schievink@ferrous-systems.com>
Copy link
Member

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the test.sh and as far as I could understand you translated it properly. Can we delete the old file then?

Also I discovered another small nit while looking through the code xD

xtask/src/main.rs Outdated Show resolved Hide resolved
@spookyvision
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 8, 2021

Build succeeded:

@bors bors bot merged commit 704bee6 into main Apr 8, 2021
@bors bors bot deleted the issue-336-ci-xtasks branch April 8, 2021 15:28
@Urhengulas
Copy link
Member

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

turn CI logic into xtasks
3 participants