Add auto-disabling/enabling to the custom test runner #161

baioc · 2021-08-24T15:17:41Z

This PR closes #145 and adds optional state tracking to the custom test runner added in #147. It uses a MySQL-compatible DB to track test history and will automatically skip unit tests marked as auto-disabled in the DB provided through the --db CLI option.

The default behavior is to only read from the test DB. Writes require a --commit option with a hash used as a unique identifier for this test run. This is meant to be used with git revision hashes when running this on the CI tool. When writing is enabled, all test results are logged and the auto-disabler/enabler runs.

Auto-enabling is quite simple: any passing test which was previously auto-disabled will be re-enabled. Of course, if the test was disabled it wouldn't normally be executed, so a --run-disabled command line switch was added to force those tests to run.

Auto-disabling takes place after the current run. It will auto-disable a test iff it failed the last 3 times it ran. This criterion is currently hard-coded but should be easy to change in the future.

This PR also includes an SQL script to initialize an empty DB with the table schemas assumed by the implementation.

baioc · 2021-08-24T15:29:40Z

@vmagro Could you take a look at this?

Although I tested it using cargo to get the new postgres dependency, it does not currently build in antlir since that's missing.
I think Postgres pulls in a bunch of other stuff (from tokio, for instance), so if that's too heavy we could switch back to SQLite.

When running this with a live Postgres server, we would need to pass in credentials in the CLI as part of the connection string. I don't have much experience dealing with this so let me know if this isn't the safest way to do it.
One possible issue is running this with the correct credentials on the CI and the DB server goes down for some reason. The program will crash, but the error message might just leak the credentials which were used.

tools/testinfra/runner/src/buck_test.rs

tools/testinfra/runner/src/main.rs

vmagro · 2021-08-24T17:32:53Z

tools/testinfra/runner/src/main.rs

+    conn: Option<String>,
+
+    /// Commit SHA-1 used to update the test DB on stateful runs
+    #[structopt(long = "commit", requires("conn"))]


Why not use --revision for the cli arg?

As it is currently, the CLI help message suggests using this option as --commit <revision>, which I kind of prefer over --revision <revision>. Also, I think "commit" makes it pretty clear we're writing to the DB, whereas "revision" not so much.

tools/testinfra/runner/src/main.rs

vmagro · 2021-08-24T17:43:34Z

tools/testinfra/runner/src/main.rs

    list: bool,

    /// Path to generated test report in JUnit XML format
    #[structopt(long = "xml")]
    report: Option<PathBuf>,

+    /// Connection string of the DB used in stateful test runs


This isn't entirely true though? Ideally we could have a default readonly connection so that non-recording runs (eg local users) can also get the list of disabled tests. Or are you planning to solve that separately?

If the disabled test feature only work on CI, that is fine for now, but it would be nice for non-stateful runs to still get the readonly info from the db

I was using the term "stateful" to refer to any execution of the test runner that uses the DB.
The default behavior is read-only and writes are only enabled when the --commit option is provided.

Summary: Needed for the new test state tracker tool the MLH Fellow is working on Differential Revision: D30546653 fbshipit-source-id: 1a983c671532000dc7a6a30b17080bdb6ff91e0c

baioc · 2021-09-07T06:33:17Z

After rebasing onto #162, this now uses MySQL instead of Postgres and I believe most of your previous comments were addressed @vmagro. As far as I'm aware, all functionalities described in this PR are working.

That said, when moving it as a local project (using Cargo) to the buck-based build inside the project's source tree, I'm getting linking errors against the indirect dependencies openssl and flate2.
For a working cargo build using the same third-party dependency versions we have in Antlir, use this Cargo.toml.

How to test this once it is built:

Add an external_runner option in the [test] section of .buckconfig, set it to the path to the test runner binary.
To check if the config is working, try appending -- --help to the buck test command line.
Run some Python and/or Rust and/or Shell tests. Other types are not yet supported. JUnit test reports are created with the optional --xml <path> CLI switch.
To set up the test initial state, add these schemas to a new database (the previous steps should work without any DB).
Append -- --db "mysql://<user>:<password>@<host>/<database>" to the buck test command line, replacing <user>, <password>, <host> and <database> with the appropriate information.
Find one or more failing tests. For testing purposes I've been forcing some tests to fail.
Add --commit <some_hash> to the command line. This enables writing results to the test DB.
After 3 runs with different commit revisions, those failing tests will be disabled. A fourth run should have 0 fails.
Add the --run-disabled CLI option to see those tests running again. If they pass and writes were enabled, they will be automatically re-enabled.

vmagro

Nice!
I'll import this as-is (and just make the changes I suggested here for you).
I'll make the ci action build the runner with Cargo first, so that we can land this before I fix the linker issues you ran into

tools/testinfra/runner/src/main.rs

vmagro · 2021-09-08T15:40:04Z

tools/testinfra/runner/src/main.rs

@@ -226,6 +295,129 @@ fn report<P: AsRef<Path>>(tests: Vec<TestResult>, path: P) -> Result<()> {
    return Ok(());
 }

-fn xml_escape_text(unescaped: String) -> String {
+fn xml_escape_text(unescaped: &String) -> String {


&str would be more conventional, and String/&String can be dereferenced as a &str

facebook-github-bot · 2021-09-21T20:28:37Z

@vmagro has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

vmagro · 2024-05-08T21:10:34Z

Sadly all of this is incompatible with buck2 :( But I think some of the details might still be useful to incorporate later

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 24, 2021

vmagro reviewed Aug 24, 2021

View reviewed changes

tools/testinfra/runner/src/buck_test.rs Outdated Show resolved Hide resolved

vmagro requested changes Aug 24, 2021

View reviewed changes

vmagro and others added 8 commits August 25, 2021 11:08

antlir oss: add mysql_async as dep of test runner

b769cd5

Summary: Needed for the new test state tracker tool the MLH Fellow is working on Differential Revision: D30546653 fbshipit-source-id: 1a983c671532000dc7a6a30b17080bdb6ff91e0c

Apply diffs from cargo-based test-runner

dd6c47b

Dump SQL table creation script

c21dfd8

List postgres dependency

b96a155

Fix small details

316ea56

Migrate to MySQL

ca2dc0f

Let BUCK = TARGETS

431cf9b

Try to work around the older mysql crate

25d025c

baioc force-pushed the stateful-testing branch from df8792e to 25d025c Compare August 27, 2021 03:49

baioc added 2 commits September 7, 2021 02:02

Better understand the mysql_async API

88883f7

Fix test re-enabling run

da74499

vmagro approved these changes Sep 8, 2021

View reviewed changes

vmagro closed this May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add auto-disabling/enabling to the custom test runner #161

Add auto-disabling/enabling to the custom test runner #161

baioc commented Aug 24, 2021 •

edited

Loading

baioc commented Aug 24, 2021

vmagro Aug 24, 2021

baioc Aug 26, 2021

vmagro Aug 24, 2021

baioc Aug 26, 2021

baioc commented Sep 7, 2021 •

edited

Loading

vmagro left a comment

vmagro Sep 8, 2021

facebook-github-bot commented Sep 21, 2021

vmagro commented May 8, 2024

Add auto-disabling/enabling to the custom test runner #161

Add auto-disabling/enabling to the custom test runner #161

Conversation

baioc commented Aug 24, 2021 • edited Loading

baioc commented Aug 24, 2021

vmagro Aug 24, 2021

Choose a reason for hiding this comment

baioc Aug 26, 2021

Choose a reason for hiding this comment

vmagro Aug 24, 2021

Choose a reason for hiding this comment

baioc Aug 26, 2021

Choose a reason for hiding this comment

baioc commented Sep 7, 2021 • edited Loading

vmagro left a comment

Choose a reason for hiding this comment

vmagro Sep 8, 2021

Choose a reason for hiding this comment

facebook-github-bot commented Sep 21, 2021

vmagro commented May 8, 2024

baioc commented Aug 24, 2021 •

edited

Loading

baioc commented Sep 7, 2021 •

edited

Loading