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

Iterate on blacklisting support #186

Closed
aidanhs opened this issue Feb 26, 2018 · 7 comments
Closed

Iterate on blacklisting support #186

aidanhs opened this issue Feb 26, 2018 · 7 comments

Comments

@aidanhs
Copy link
Member

aidanhs commented Feb 26, 2018

Following up on #100 and the PR that adds a blacklist (many thanks for the great work @pietroalbini!), I tried this on a box:

diff --git a/config.toml b/config.toml
index 8e84c56..c8dc651 100644
--- a/config.toml
+++ b/config.toml
@@ -12,5 +12,25 @@
 [crates]
 # crate_name = { option = true }
 
+# Spurious failures
+
+# Consistent test failures
+accel = { skip-tests = true }
+aluminum = { skip-tests = true }
+
+# Consistent build failures
+acorn = { skip = true }
+actors = { skip = true }
+
 [github-repos]
 # "org_name/repo_name" = { option = true }
+
+# Spurious failures
+
+# Consistent test failures
+"stewart/rff" = { skip-tests = true }
+"wwared/rgb" = { skip-tests = true }
+
+# Consistent build failures
+"zacstewart/rush" = { skip = true }
+"zmbush/megaprompt" = { skip = true }

(just some random consistent failures I noticed)

and got these results - http://cargobomb-reports.s3.amazonaws.com/pr-46785/index.html

Observations:

  • items that are completely skipped have status 'unknown' in the report - maybe should just be 'skipped'?
  • items that have their tests skipped are listed as 'test-pass' - maybe should be 'test-skipped' in the report to avoid masking this?
  • accel is an oddity - it successfully builds the library, but fails to build the tests (and doesn't try to run them because it's listed as 'skip-tests')...so why has this been picked up as test-pass!?
    • We clearly need better detection of failure when building tests - if they've not built the first time, another try won't help.
    • We possibly need another level of skipping to indicate stopping just after tests are built.
@Mark-Simulacrum
Copy link
Member

I've always seen "build" as both cargo build and cargo test --no-run - so I don't think we need more than just build vs. test distinction.

@pietroalbini
Copy link
Member

Items that are completely skipped have status 'unknown' in the report - maybe should just be 'skipped'?

Done, sent PR #187.

Items that have their tests skipped are listed as 'test-pass' - maybe should be 'test-skipped' in the report to avoid masking this?

Also in #187. By the way, this was always the case with the build-only crater run: successful builds were marked as "test-pass", and I kept this behavior in the original PR.

accel is an oddity - it successfully builds the library, but fails to build the tests (and doesn't try to run them because it's listed as 'skip-tests')...so why has this been picked up as test-pass!?

I'll investigate tomorrow.


By the way, another thing I noticed after the PR was merged is that currently crates are just skipped in the run phase, but their data is still prepared in the prepare-ex phase, wasting time.

@pietroalbini
Copy link
Member

accel is an oddity - it successfully builds the library, but fails to build the tests (and doesn't try to run them because it's listed as 'skip-tests')...so why has this been picked up as test-pass!?

That was an existing bug that showed thanks to skip-tests, fixed in #189.

By the way, another thing I noticed after the PR was merged is that currently crates are just skipped in the run phase, but their data is still prepared in the prepare-ex phase, wasting time.

PR sent for this: #188.

@aidanhs
Copy link
Member Author

aidanhs commented Mar 3, 2018

I think I'm ready to give this another shot on the next crater run - only #188 from the flurry of PRs @pietroalbini created is still open, and that's just for speedup purposes.

@aidanhs
Copy link
Member Author

aidanhs commented Mar 14, 2018

A few days ago I started another run (including #188) with the config.toml per the OP. Sorry, I forgot to comment here.

@aidanhs
Copy link
Member Author

aidanhs commented Mar 15, 2018

http://cargobomb-reports.s3.amazonaws.com/pr-48270/index.html is the result - looks awesome! I'll create a PR based on consistent failures across the last few runs and we can speed up our crater runs!

One note about accel - by bundling build and test --no-run together we are losing some information from crates that only succeed in the first. It's not that important since I don't see there being many like this, but I'll note it down in a low priority spin-off issue.

@aidanhs
Copy link
Member Author

aidanhs commented Mar 29, 2018

This issue is done.

@aidanhs aidanhs closed this as completed Mar 29, 2018
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

No branches or pull requests

3 participants