test suite: Do not generate identical expectation files #598

petrochenkov · 2020-10-12T20:57:45Z

For C code the test suite runs cbindgen multiple times with different settings of --cpp-compat and --style.
These runs may generate identical code if the original Rust file doesn't have language constructions relevant to those options.

With this PR we no longer save duplicate cbindgen outputs into files and don't run C or C++ compiler on them.
This saves some testing time (around 10% on my machine) and makes reviewing of new test cases easier, new tests are less likely to add a wall of text half of which is duplicated.

It will also help to add less test files in #590 (--style makes sense for Cython too).

emilio

So I'm not sure how I feel about this, I tend to think tests should try to be as dumb as possible... On the other hand it's nice that this removes almost 3k lines (and I guess this is worse for cython and that's why you want this?)

Either way I have a few nits etc.

emilio · 2020-10-14T16:52:57Z

tests/tests.rs

+    if cbindgen_outputs.contains(&cbindgen_output) {
+        // We already generated an identical file previously.
+        return;
+    } else {


nit: No else after return.

I'm not sure what do you mean.

Restructured the code slightly to avoid return.

emilio · 2020-10-14T16:55:49Z

tests/tests.rs

        cbindgen_path,
        path,
        &generated_file,
        language,
        cpp_compat,
        style,
    );
+    if cbindgen_outputs.contains(&cbindgen_output) {


I think this should assert that generated_file doesn't exist in CBINDGEN_TEST_VERIFY mode, and remove it if it does otherwise.

emilio · 2020-10-14T16:56:56Z

tests/tests.rs

-    command.arg("-o").arg(output);
-
-    if env::var("CBINDGEN_TEST_VERIFY").is_ok() {
-        command.arg("--verify");


We should remove this flag if we're doing this, probably.

It's a reasonable thing to use on CI if cbindgen is used offline rather than from a build script, I'd keep it.
(Also it's a public cbindgen's interface after all, someone else is probably using it.)

emilio · 2020-10-14T16:57:42Z

tests/tests.rs

-        for cpp_compat in &[true, false] {
+    // Run tests in deduplication priority order. C++ compatibility tests are run first,
+    // otherwise we would lose the C++ compiler run if they were deduplicated.
+    let mut cbindgen_outputs = HashSet::new();


Hmm, I wonder how much peak memory does this use, guess not too much given the size of the test-cases so no big deal.

Yeah, the files are small, the largest one is tests\expectations\destructor-and-copy-ctor.cpp with 13kb, it's nothing.

emilio · 2020-10-14T16:59:01Z

tests/tests.rs

+            fs::write(&generated_file, &cbindgen_output).unwrap();
+        }
+        cbindgen_outputs.insert(cbindgen_output);
+    }

    compile(


Hmm, I feel a bit uneasy about this work being skipped generally... But I guess it's mostly fine.

We are only skipping identical compiler runs ("compile as C") on files with identical contents, since we are not deduplicating "accross languages".

petrochenkov · 2020-10-17T12:43:44Z

@emilio

So I'm not sure how I feel about this, I tend to think tests should try to be as dumb as possible...

So, I had an idea how to increase "obviousness" of the deduplication and also improve the test suite in general.

Right now expectation files generated from the same Rust file are located far from each other, in different directories (tag, both, etc). This directory structure was introduced in #112 and not discussed.

I suggest to place different outputs from the same file next to each other instead, and make them differ only by extensions (this is how it's done in rustc test suite).
I also suggest to add suffixes in the order of deduplication priority, so it's clear that if my_test.foo.bar is missing, then it should be identical to mytest.foo.

So, the outputs from annotation.rs would be:

annotation.c
annotation.both.c
annotation.tag.c
annotation.nocompat.c
annotation.nocompat.both.c
annotation.nocompat.tag.c

and deduplicated outputs could look like

# c++ compat mode is not relevant to this test (but we are still running both c and c++ compiler!)
annotation.c
annotation.both.c
annotation.tag.c

or

# struct definition styles are not relevant to this test
annotation.c
annotation.nocompat.c

petrochenkov · 2020-10-17T13:58:01Z

The suggestion is implemented in #602.

I don't quite like the .nocompat part though, since cpp-compat is not the default.
Perhaps

# c++ compat mode is not relevant to this test (but we are still running both c and c++ compiler!)
annotation.compat.c
annotation.compat.both.c
annotation.compat.tag.c

after deduplication is clear enough even if we deduplicate in favor of a longer suffix.

petrochenkov force-pushed the duptest branch 7 times, most recently from ea3a6ee to c99e355 Compare October 13, 2020 20:55

emilio reviewed Oct 14, 2020

View reviewed changes

petrochenkov force-pushed the duptest branch from c99e355 to 1a43e86 Compare October 17, 2020 13:09

petrochenkov mentioned this pull request Oct 17, 2020

test suite: Use suffixes instead of directories for test variants #602

Merged

petrochenkov added 2 commits October 19, 2020 22:51

test suite: Do not generate identical expectation files

f65fbfc

test suite: Remove existing duplicate expectation files

30090f9

petrochenkov force-pushed the duptest branch from 1a43e86 to 30090f9 Compare October 19, 2020 19:52

emilio approved these changes Oct 20, 2020

View reviewed changes

emilio merged commit 050863a into mozilla:master Oct 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test suite: Do not generate identical expectation files #598

test suite: Do not generate identical expectation files #598

petrochenkov commented Oct 12, 2020

emilio left a comment

emilio Oct 14, 2020

petrochenkov Oct 17, 2020

petrochenkov Oct 17, 2020

emilio Oct 14, 2020

petrochenkov Oct 17, 2020

emilio Oct 14, 2020

petrochenkov Oct 17, 2020

emilio Oct 14, 2020

petrochenkov Oct 17, 2020

emilio Oct 14, 2020

petrochenkov Oct 17, 2020

petrochenkov commented Oct 17, 2020

petrochenkov commented Oct 17, 2020

test suite: Do not generate identical expectation files #598

test suite: Do not generate identical expectation files #598

Conversation

petrochenkov commented Oct 12, 2020

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Oct 17, 2020

petrochenkov commented Oct 17, 2020