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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for .toml tests #3500

Merged
merged 10 commits into from Jul 20, 2020
Merged

Conversation

thisismiller
Copy link
Contributor

@thisismiller thisismiller commented Jul 12, 2020

This is part 3 of #3459, which turned out to be better to do before part 2 of what I had outlined there.

This introduces a new dependency, toml11, into CMake, and then uses it to implement a TestSpec-equivalent TOML reader. I have no idea if I did this right, but the code compiles. 馃し. TOML has typed values, and TestSpec treats all values as strings. To make this change easier to do in pieces, all TOML values are currently turned back into strings, and then placed into the TestSpec struct. A later PR will look into carrying this typedness into the workload framework, so that type mismatches can be reported with toml11's superior error messages[1].

The largest changes to note between what is in testspec files and what TOML accepts is that (1) strings must be quoted (and I'd suggest always using single quotes, because they don't do backslash escaping within them) (2) doubles need to be of the form 0.8 and not .8. Aside from that, it's a mostly straightforward implementation. I've included the python script that I wrote to do the transformation of .txt into .toml, and it also pretty prints/applies a nice formatting to the result.

I verified that TestHarness doesn't need any changes, so Joshua should still work. TestRunner also doesn't require changes, and ctest works as expected. Any other code that currently generates testspec files (e.g. Circus) should still work, as long as it outputs a file which ends in .txt.


diff --git a/tests/fast/CloggedSideband.toml b/tests/fast/CloggedSideband.toml
index 8c744f3fb..139a3aeae 100644
--- a/tests/fast/CloggedSideband.toml
+++ b/tests/fast/CloggedSideband.toml
@@ -13,7 +13,7 @@ testTitle = 'CloggedCausalConsistencyTest'
     [[test.workload]]
     testName = 'RandomClogging'
     testDuration = 30.0
-    scale = 0.1
+    scale = .1
     clogginess = 2.0
 
     [[test.workload]]

Then when running the test, the output will look like:

$ ./build/foundationdb/linux/bin/fdbserver -r simulation -b on -s 1 -f foundationdb/tests/fast/CloggedSideband.toml
Random seed is 1...
Datacenter 0: 4/12 machines, 1/1 coordinators
Datacenter 1: 4/12 machines, 0/1 coordinators
Datacenter 2: 4/12 machines, 0/1 coordinators
[error] bad float: invalid format
 --> foundationdb/tests/fast/CloggedSideband.toml
    | 
 16 |     scale = .1
    |             ^-- integer part required before this
    | 
Hint: pass: +1.0, -2e-2, 3.141_592_653_589, inf, nan
Hint: fail: .0, 1., _1.0, 1.0_, 1_.0, 1.0__0
Unseed: 51505
Elapsed: 1.000000 simsec, 0.023431 real seconds
2 SevError events logged

Making getOption take a template parameter of the type in workloads will extend this error reporting to type mismatches between toml file and what the workload expects.

I was hoping to add a similar looking warning if global-level or test-level params are used at the wrong level, but that will be blocked on toml11#124.

This separates the file format and how to read it from how to apply a key/value
to a TestSpec.  This will allow us to reuse the same code when implementing a
TOML parser later.
This includes build/txt-to-toml.py which did the rewrites, and
can be used to rewrite other no-in-tree test spec files to toml.

I didn't touch status or restarting tests yet.  Restarting will be handled
later.  It turns out that I don't understand how status tests work.
@xumengpanda
Copy link
Contributor

@fdb-build ok to build

@xumengpanda
Copy link
Contributor

@fdb-build ok to test.

build/txt-to-toml.py Outdated Show resolved Hide resolved
@thisismiller
Copy link
Contributor Author

I ran

for testfile in $(ls foundationdb/tests/{fast,slow,rare}/*.toml | sed s/.toml//); do
    echo $testfile
    diff -u -a <(cd txt_tests; ../build/foundationdb/linux/bin/fdbserver -r simulation -b on -s 1 -f ../$testfile.txt --crash --logsize=0B | grep Unseed:)\
               <(cd toml_tests; ../build/foundationdb/linux/bin/fdbserver -r simulation -b on -s 1 -f ../$testfile.toml --crash --logsize=0B | grep Unseed:)
done

Which walks through the .txt and .toml versions of each test, and compares their unseed to assert that the TOML versions of the tests do the exact same thing that the .txt versions of the test did.

There's a few mismatches:

foundationdb/tests/fast/CacheTest
foundationdb/tests/fast/CloggedSideband
foundationdb/tests/fast/InventoryTestSomeWrites
foundationdb/tests/fast/MoveKeysCycle
foundationdb/tests/rare/ClogUnclog
foundationdb/tests/rare/CycleRollbackClogged
foundationdb/tests/slow/CloggedCycleTest
foundationdb/tests/slow/CloggedStorefront
foundationdb/tests/slow/ClogWithRollbacks
foundationdb/tests/slow/MoveKeysClean

Staring at the results, there's not that much that's actually different. In the TOML version, the workload RPC shows up a few milliseconds of sim time later. All of these tests use 0.1 or 0.8 in their testspec, which because I convert the TOML values back to a string, get sent across the wire as 0.1000000000001 or 0.80000000000004. Evan once commented that packet delays in Simulation are a function of packet size, so this change in string representation could explain the unseed change. The fact that most tests don't seem to exhibit this behavior also seems to support this theory.

I can't seem to find the code in simulation that does a delay as a function of packet size though?

@thisismiller thisismiller marked this pull request as ready for review July 13, 2020 03:19
@jzhou77
Copy link
Contributor

jzhou77 commented Jul 13, 2020

@fdb-build test this please

@xumengpanda xumengpanda merged commit e069a15 into apple:master Jul 20, 2020
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.

None yet

3 participants